[Feature] Simplify Operator ID #1981
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the operator image discovery process by introducing a new util.Image struct that encapsulates both the base image and an optional status image. This replaces the previous string-based image handling and streamlines how the operator determines which image to use based on the deployment's image discovery mode.
- Introduced a new
util.Imagestruct to handle operator image discovery with both base image and optional status image - Refactored
NewOperatorsignature to acceptutil.Imageinstead of a string throughout the codebase - Updated the image discovery mechanism in
cmd/cmd.goto fetch both spec and status images - Deprecated the
--image.discovery.statusflag as the discovery method has changed
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/image.go | New file defining the Image struct with Get() method for hash-based image selection |
| pkg/operatorV2/operator.go | Updated to use util.Image type and call Get(true) for image retrieval |
| pkg/operator/operator.go | Changed OperatorImage field to Image of type util.Image |
| cmd/cmd.go | Refactored image discovery to populate both Image and StatusImage fields; deprecated related flag |
| pkg/deployment/context_impl.go | Updated GetOperatorImage() to use Image.Get() with deployment-specific discovery mode |
| Multiple test files | Updated test initialization to use util.Image{Image: "..."} syntax |
| Documentation files | Removed deprecated --image.discovery.status flag from CLI documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ajanikow
force-pushed
the
feature/simplify_operator_id
branch
from
2c683db to
3c6368b
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/cmd.go
Outdated
| operatorImageDiscovery struct { | ||
| timeout time.Duration | ||
| timeout time.Duration | ||
| //deprecated: Do not use this flag, as discovery method changed |
There was a problem hiding this comment.
Corrected spelling of 'deprecated' to 'Deprecated' in comment. Deprecation notices should start with 'Deprecated:' (capitalized and followed by a colon) to follow Go documentation conventions.
| //deprecated: Do not use this flag, as discovery method changed | |
| // Deprecated: Do not use this flag, as discovery method changed |
| f.MarkDeprecated("operator.metrics-exporter-image", "Value is not used anymore"), | ||
| f.MarkDeprecated("operator.arango-image", "Value is not used anymore"), | ||
| f.MarkDeprecated("scope", "Value is not used anymore"), | ||
| f.MarkDeprecated("image.discovery.status", "Value fetched from the Operator Spec"), |
There was a problem hiding this comment.
The deprecation message 'Value fetched from the Operator Spec' is unclear. It doesn't explain what users should do or why the flag is deprecated. Consider a more descriptive message like 'Image discovery method is now determined by the deployment's ImageDiscoveryMode specification' to help users understand the change.
| f.MarkDeprecated("image.discovery.status", "Value fetched from the Operator Spec"), | |
| f.MarkDeprecated("image.discovery.status", "Image discovery method is now determined by the deployment's ImageDiscoveryMode specification"), |
No description provided.