Skip to content

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

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.

Sign up for GitHub

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

Open
olzemal wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/ocm/cluster-workflow-template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ spec:
consumers:
- 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 •
edited
Loading

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.

@olzemal olzemal Jun 30, 2026

Copy link
Copy Markdown
Collaborator 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?

@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.

@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:443 → dst.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.

@trevex trevex Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Honestly not sure...

It was most likely due to port but might be because of ocm CLI quirkiness, so if it still works with explicit port we should include the port.

@coderabbitai coderabbitai Bot Jul 14, 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.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

credentials:
- type: Credentials
properties:
Expand Down Expand Up @@ -205,7 +205,7 @@ spec:
consumers:
- identity:
type: OCIRegistry
hostname: $(echo '{{workflow.parameters.dstRemoteURL}}' | cut -d : -f 1)
hostname: $(echo '{{workflow.parameters.dstRemoteURL}}' | cut -d / -f 1 | cut -d : -f 1)
credentials:
- type: Credentials
properties:
Expand Down
Loading