chore(constants): move VM facts keys to shared package#19542
chore(constants): move VM facts keys to shared package#19542dashrews78 wants to merge 2 commits intomasterfrom
Conversation
…ckage The facts key constants (GuestOSKey, DescriptionKey, etc.) were defined in the sensor dispatcher package, making them inaccessible to central without creating a bad dependency direction. Moved them to pkg/virtualmachine/facts.go so both sensor and central can reference the same constants instead of using string literals. Partially generated by AI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In files that import both
pkg/virtualmachineandsensor/common/virtualmachine, consider using a consistent alias (e.g. alwayspkgVMfor the shared package) to make it immediately clear which package a given identifier comes from and avoid confusion when reading the tests. - Since
UnknownGuestOSis now a shared constant, it may be worth clarifying its intended semantics in the newfacts.go(e.g. whether it is a user-facing string vs. purely internal sentinel) so other callers don’t accidentally treat it as a real OS name.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In files that import both `pkg/virtualmachine` and `sensor/common/virtualmachine`, consider using a consistent alias (e.g. always `pkgVM` for the shared package) to make it immediately clear which package a given identifier comes from and avoid confusion when reading the tests.
- Since `UnknownGuestOS` is now a shared constant, it may be worth clarifying its intended semantics in the new `facts.go` (e.g. whether it is a user-facing string vs. purely internal sentinel) so other callers don’t accidentally treat it as a real OS name.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Address Sourcery review feedback: - Use pkgVM alias consistently for pkg/virtualmachine in dispatcher files that also import sensor/common/virtualmachine - Document UnknownGuestOS as a user-facing default value Partially generated by AI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Images are ready for the commit at 3ebf14e. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19542 +/- ##
=======================================
Coverage 49.25% 49.26%
=======================================
Files 2727 2727
Lines 205788 205788
=======================================
+ Hits 101362 101375 +13
+ Misses 96890 96881 -9
+ Partials 7536 7532 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vikin91
left a comment
There was a problem hiding this comment.
LGTM!
Simple with limited scope, quick and easy to review :)
|
/retest |
|
@dashrews78: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
The facts key constants (GuestOSKey, DescriptionKey, etc.) were defined
in the sensor dispatcher package, making them inaccessible to central
without creating a bad dependency direction. Moved them to
pkg/virtualmachine/facts.go so both sensor and central can reference
the same constants instead of using string literals.
Partially generated by AI
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
User-facing documentation
Testing and quality
Automated testing
How I validated my change
The updates tests should be sufficient here.