-
Notifications
You must be signed in to change notification settings - Fork 39
Eliminates direct access to old-style repo configs #2332
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThree files are modified to refactor how repository configurations are sourced and represented. Instead of directly accessing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
This change eliminates direct access to old-style repo configurations (group_config.repos or group_config['repos']) by using runtime.repos which handles both old-style and new-style repo configs. Changes: - Add to_dict() method to Repo class to export configuration as dict - Refactor calculate_config_digest() to use runtime.repos[].to_dict() - Refactor images_streams_gen_buildconfigs() to use runtime.repos This simplifies code and ensures consistent repo handling across both old-style and new-style configurations.
1da7099 to
fb2d13d
Compare
|
@vfreex: The following test 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. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
doozer/doozerlib/repos.py (1)
210-212: LGTM! Clean API addition for repo configuration export.The new
to_dict()method provides a clean interface for exporting repository configuration as a dictionary, enabling the refactoring in other files. The delegation toself._data.primitive()correctly handles recursive conversion of Model objects to plain dicts.Optional: Consider more specific return type annotation
The return type could be more specific for better type safety:
- def to_dict(self) -> Dict: + def to_dict(self) -> Dict[str, Any]:This matches the pattern used in other similar methods and provides clearer type information.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
doozer/doozerlib/cli/images_streams.pydoozer/doozerlib/image.pydoozer/doozerlib/repos.py
🧰 Additional context used
🧬 Code graph analysis (2)
doozer/doozerlib/cli/images_streams.py (2)
artcommon/artcommonlib/model.py (1)
Model(116-155)doozer/doozerlib/repos.py (2)
to_dict(210-212)items(482-483)
doozer/doozerlib/repos.py (1)
doozer/doozerlib/lockfile.py (3)
to_dict(83-98)to_dict(134-145)to_dict(192-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (2)
doozer/doozerlib/image.py (1)
633-634: LGTM! Clean refactoring to unified repo access.The change correctly replaces direct
group_config.reposaccess withruntime.repos, which handles both old-style and new-style configurations. The use ofto_dict()provides a clean API for exporting repo configurations.doozer/doozerlib/cli/images_streams.py (1)
518-520: LGTM! Well-documented refactoring with backward compatibility.The change correctly switches to
runtime.reposwhile maintaining compatibility with existing code that expects dot-notation access. The Model wrapper is necessary since downstream code (e.g., lines 596-600) accesses properties likerepo_desc.confandrepo_conf.ci_alignment. The comments clearly explain both the repo source change and the Model wrapping rationale.
This change eliminates direct access to old-style repo configurations (group_config.repos or group_config['repos']) by using runtime.repos which handles both old-style and new-style repo configs.
Changes:
This simplifies code and ensures consistent repo handling across both old-style and new-style configurations.