Convert IDMS/ICSP into registries.conf #674
Conversation
📝 WalkthroughWalkthroughThis update introduces new variables and tasks to the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 4m 21s |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ansible-collection-redhatci-ocp.spec (1)
57-59: Document changelog for 2.3.EPOCH release
The new entry records the version bump and role updates. Consider briefly mentioning theimage-sourcesconversion feature to give more context in the changelog.roles/acm/utils/README.md (1)
65-82: Add image-sources usage example
The new example correctly demonstrates theimage-sourcestask. For extra clarity, you could show a snippet of the resultingutils_acm_registriesvariable.roles/acm/utils/tasks/image-sources.yml (2)
11-22: Simplify the Jinja merging logic
Building a dict via manual loops andupdate()is fragile and prints unwantedNone. Consider Ansible’scombinefilter to merge your mirror maps:- combined_mirrors: |- - {%- set c = dict() %} - {%- for mirror in hub_mirrors + spoke_mirrors %} - {%- set s = mirror.source %} - {%- set m = mirror.mirrors %} - {%- if c.get(s) %} - {{ c.update({s: c[s] + m}) }} - {%- else %} - {{ c.update({s: m}) }} - {%- endif %} - {%- endfor %} - {{ c }} + combined_mirrors: "{{ hub_mirrors | combine(spoke_mirrors, recursive=True) }}"This yields a merged dict directly and avoids side-effects.
28-39: Deterministic ordering & cleanup
Iterating overcombined_mirrors.keys() | listcan produce non-deterministic TOML block order. Consider sorting the keys for reproducible output:{% for registry in combined_mirrors.keys() | sort %}Also, you already filter mirrors with
unique—nice touch.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ansible-collection-redhatci-ocp.spec(2 hunks)galaxy.yml(1 hunks)roles/acm/utils/README.md(2 hunks)roles/acm/utils/defaults/main.yml(1 hunks)roles/acm/utils/meta/argument_specs.yml(1 hunks)roles/acm/utils/tasks/image-sources.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Sanity Check (stable-2.17)
- GitHub Check: Sanity Check (stable-2.9)
🔇 Additional comments (8)
galaxy.yml (1)
12-12: Bump collection version to 2.3.0
Version bump from2.2.0to2.3.0aligns with the newacm.utilsenhancements. Ensure this is reflected across all metadata and documentation.roles/acm/utils/defaults/main.yml (1)
4-7: Add default for unqualified search registries
The newutils_unqualified_search_registriesdefault provides sensible fallback registries for unqualified image lookups. It matches the argument spec and README.ansible-collection-redhatci-ocp.spec (1)
6-6: Update RPM spec version to 2.3.EPOCH
This aligns the RPM spec version with the collection bump for the newly addedimage-sourcesfeature inacm.utils.roles/acm/utils/README.md (1)
8-16: Document new image-sources variables in table
The addition ofutils_unqualified_search_registriesis well aligned with the defaults and argument specs, and the table formatting is clear.roles/acm/utils/meta/argument_specs.yml (1)
41-64: Defineimage-sourcesargument spec
The spec enforces required parameters (utils_registry,utils_imagesource_file) and provides a default forutils_unqualified_search_registries. Descriptions and types are clear and consistent with the role’s implementation.roles/acm/utils/tasks/image-sources.yml (3)
1-3: Header and task declaration look good
Using---and a top-level- name:is valid for a tasks file in a role.
23-27: Inspect indentation of theset_factblock
The literal block (|-) will preserve all leading spaces, which may lead to unexpected whitespace in your generatedregistries.conf. Please verify howutils_acm_registriesis consumed downstream to ensure the indent is trimmed or acceptable.
41-47: Validate availability of theurlsplitfilter
You use{{ utils_registry | urlsplit('netloc') }}—ensure that theurlsplitJinja2 filter is provided by your Ansible version or a loaded collection (e.g.,ansible.netcommon). If it’s not, this will error out.
tonyskapunk
force-pushed
the
acm_imagesources
branch
from
a134ca7 to
e417e26
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 4m 23s |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
roles/acm/utils/meta/argument_specs.yml (4)
47-51: Clarifyutils_registryformatting.
Consider specifying if this should be a hostname (e.g.,registry.local:5000) or a full URL with protocol, and whether trailing slashes are allowed.
52-61: Improve example formatting forutils_hub_mirrors.
The description block is helpful, but the nested YAML example could use consistent indentation. You might illustrate it as:- mirrors: - mirror1 source: source1
62-67: Clarify path expectations forutils_imagesource_file.
Does this accept only absolute paths, relative to the playbook, or both? Adding a brief note will help users avoid confusion.
68-74: Align defaultutils_unqualified_search_registries.
Since this default also appears indefaults/main.yml, ensure they stay in sync or consider centralizing it to avoid drift.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ansible-collection-redhatci-ocp.spec(2 hunks)galaxy.yml(1 hunks)roles/acm/utils/README.md(2 hunks)roles/acm/utils/defaults/main.yml(1 hunks)roles/acm/utils/meta/argument_specs.yml(1 hunks)roles/acm/utils/tasks/image-sources.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- galaxy.yml
- roles/acm/utils/defaults/main.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- roles/acm/utils/README.md
- ansible-collection-redhatci-ocp.spec
- roles/acm/utils/tasks/image-sources.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Sanity Check (stable-2.17)
- GitHub Check: Sanity Check (stable-2.9)
🔇 Additional comments (1)
roles/acm/utils/meta/argument_specs.yml (1)
41-46: Newimage-sourcesargument spec follows existing patterns and explains its purpose clearly.
Well structured and consistent with the other argument specs.
This is a functionality that is required to deploy clusters with a different version than the hub. It is used by the spoke clusters in disconnected mode.
tonyskapunk
force-pushed
the
acm_imagesources
branch
from
e417e26 to
2d7679c
Compare
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 4m 28s |
|
from change #674: |
|
from change #674: |
|
from change #674: |
SUMMARY
This is a functionality that is required to deploy clusters with a different version than the hub. It is used by the spoke clusters in disconnected mode.
ISSUE TYPE
Tests
Build-depends: https://softwarefactory-project.io/r/c/dci-openshift-agent/+/33754