Skip to content

fix: strip any sub-paths for registries in ocmconfig#424

Open
olzemal wants to merge 1 commit into
mainfrom
fix/handle-ocm-repository-with-path
Open

fix: strip any sub-paths for registries in ocmconfig#424
olzemal wants to merge 1 commit into
mainfrom
fix/handle-ocm-repository-with-path

Conversation

@olzemal

@olzemal olzemal commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

What

Handling registries like example.com/my-repo. OCM expects credentials to be configured for just example.com. We did not strip any possible subpaths prior to this change.

Checklist

  • Tests added/updated
  • No breaking changes (or upgrade path documented above)
  • Readable commit history (squashed and cleaned up as desired)
  • AI code review considered and comments resolved

Summary by CodeRabbit

  • Bug Fixes
    • Corrected the registry hostname values used in the OCM cluster workflow template for both source and destination container registries.
    • This helps ensure generated credentials configuration points to the proper registry locations during workflow execution.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Two hostname: field lines in the embedded bash scripts within examples/ocm/cluster-workflow-template.yaml are modified — one for the source registry credential config (line 123, derived from srcRemoteURL) and one for the destination registry credential config (line 208, derived from dstRemoteURL).

Changes

OCM Cluster Workflow Template

Layer / File(s) Summary
hostname lines in src/dst credential scripts
examples/ocm/cluster-workflow-template.yaml
Updates the hostname: field lines for both source (srcRemoteURL) and destination (dstRemoteURL) registry credential config generation in the embedded bash scripts.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

A rabbit hops through yaml land,
Two hostname lines get a helping hand,
Src and dst, both set just right,
Credentials config, now shining bright! 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description only includes What and Checklist; it is missing the Why, Testing, and Notes for reviewers sections from the template. Add the missing Why, Testing, and Notes for reviewers sections, and briefly describe how the change was validated.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: stripping registry sub-paths in ocmconfig.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/handle-ocm-repository-with-path

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@olzemal olzemal force-pushed the fix/handle-ocm-repository-with-path branch from fd04c59 to 886afc9 Compare June 29, 2026 12:37
@olzemal

olzemal commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@examples/ocm/cluster-workflow-template.yaml`:
- Line 123: The hostname parsing in the workflow template is stripping off the
port as well as the repo path, which breaks host matching for registries like
dst.zot:443. Update the expression that builds hostname in the affected workflow
steps so it removes only the repository sub-path from
workflow.parameters.srcRemoteURL while preserving the full host:port identity;
use the existing hostname assignment logic in the cluster workflow template as
the place to make the change and keep the port intact everywhere this pattern
appears.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ad8a25f1-81c5-4888-baba-7863357f1383

📥 Commits

Reviewing files that changed from the base of the PR and between d79e20a and 886afc9.

📒 Files selected for processing (1)
  • examples/ocm/cluster-workflow-template.yaml

- identity:
type: OCIRegistry
hostname: $(echo '{{workflow.parameters.srcRemoteURL}}' | cut -d : -f 1)
hostname: $(echo '{{workflow.parameters.srcRemoteURL}}' | cut -d / -f 1 | cut -d : -f 1)

@coderabbitai coderabbitai Bot Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve host:port when stripping the repo sub-path.

cut -d : -f 1 changes the registry identity, not just the path. That will break credential matching for registries exposed on an explicit port (for example dst.zot:443, which is already used elsewhere). This change should strip only the /repo suffix and keep any port intact.

Suggested fix
-                    hostname: $(echo '{{workflow.parameters.srcRemoteURL}}' | cut -d / -f 1 | cut -d : -f 1)
+                    hostname: $(echo '{{workflow.parameters.srcRemoteURL}}' | cut -d / -f 1)
...
-                    hostname: $(echo '{{workflow.parameters.dstRemoteURL}}' | cut -d / -f 1 | cut -d : -f 1)
+                    hostname: $(echo '{{workflow.parameters.dstRemoteURL}}' | cut -d / -f 1)

Also applies to: 208-208

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@examples/ocm/cluster-workflow-template.yaml` at line 123, The hostname
parsing in the workflow template is stripping off the port as well as the repo
path, which breaks host matching for registries like dst.zot:443. Update the
expression that builds hostname in the affected workflow steps so it removes
only the repository sub-path from workflow.parameters.srcRemoteURL while
preserving the full host:port identity; use the existing hostname assignment
logic in the cluster workflow template as the place to make the change and keep
the port intact everywhere this pattern appears.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevex do you remember why we split on :. Was it for repo:tag (does the srcRemoteURL have a tag?) or host:port?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olzemal — looking at how srcRemoteURL is used elsewhere in the template, it appears the split on : was for host:port, not repo:tag. The tag portion (:specVersion) is appended separately via the constructed component_url:

component_url={{workflow.parameters.srcRemoteURL}}/{{workflow.parameters.specRepo}}//{{workflow.parameters.specComponent}}:{{workflow.parameters.specVersion}}

So srcRemoteURL itself is expected to be something like registry.example.com, registry.example.com:443, or (after this PR) registry.example.com/my-repo — no tag component.

That's exactly why my original comment flagged the concern: the cut -d : -f 1 in the original code, and retained in this PR after cut -d / -f 1, strips the port (e.g. dst.zot:443dst.zot). If OCM does need the full host:port for credential matching on non-standard ports, that would be a problem. If OCM only ever matches on the bare hostname, then stripping the port is intentional and correct — worth confirming against the OCM documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants