-
Notifications
You must be signed in to change notification settings - Fork 23
refactor: make config/default/images.env single source of truth #1094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR refactors image configuration to use Sequence Diagram: Runtime Loading of Embedded Image ConfigurationsequenceDiagram
participant RT as Go Runtime
participant INIT as images.init()
participant FS as embed.FS (content)
participant PCF as parseConfigFile()
participant REG as images.Registry
RT->>+INIT: Executes package initialization
INIT->>+FS: ReadFile("embed/images.env")
FS-->>-INIT: []byte (fileData), error
Note right of INIT: if err != nil, panic(err)
INIT->>+PCF: parseConfigFile(fileData)
PCF->>PCF: Creates bytes.NewReader(fileData)
PCF->>PCF: Creates bufio.NewScanner(reader)
PCF->>PCF: Scans lines & parses key-value pairs
PCF-->>-INIT: map[Image]string (data), error
Note right of INIT: if err != nil, panic(err)
INIT->>+REG: Populate(data)
REG-->>-INIT:
INIT-->>-RT: Initialization complete
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @osmman - I've reviewed your changes - here's some feedback:
- Consider directly embedding
../../config/default/images.env(e.g., into a[]byte) to simplify the embedding mechanism, as this would remove thego:generate cpstep and the intermediateembed/directory.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
8792ac0 to
b5397f6
Compare
Uses go generate feature to copy images.env into embed folder. Which content will be added into embeded file system. Signed-off-by: Tomas Turek <tturek@redhat.com>
b5397f6 to
41357da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JasonPowr, osmman The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Uses go generate feature to copy images.env into embed folder. Which content will be added into embeded file system.
Summary by Sourcery
Refactor the images configuration to use config/default/images.env as the single source of truth, leveraging go generate to manage the embedded configuration
Bug Fixes:
Enhancements:
Chores: