Skip to content

fix: trim trailing slashes from the base-path#1396

Merged
frewilhelm merged 1 commit into
open-component-model:mainfrom
Skarlso:trim-trailing-slash
Dec 12, 2025
Merged

fix: trim trailing slashes from the base-path#1396
frewilhelm merged 1 commit into
open-component-model:mainfrom
Skarlso:trim-trailing-slash

Conversation

@Skarlso

@Skarlso Skarlso commented Dec 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What this PR does / why we need it

Avoids things like this:

Component:         bob.poc.sap.com/root
...
    Repository Spec:
      Base URL:  mcp-blueprints.common.repositories.cloud.sap/ocm/
...

ending up like this:

Message:               failed to list versions: failed to get store for reference: invalid reference: invalid repository "ocm//component-descriptors/bob.poc.sap.com/root"

Because, basically oras has parts := strings.SplitN(artifact, "/", 2) in it's parser which resulted in the repository section being cut as ocm//... for the full reference of mcp-blueprints.common.repositories.cloud.sap/ocm//component-descriptors/bob.poc.sap.com/root.

Which issue(s) this PR fixes

@Skarlso Skarlso requested a review from a team as a code owner December 11, 2025 15:39
frewilhelm
frewilhelm reviewed Dec 11, 2025
View reviewed changes
Comment thread bindings/go/oci/repository/repository.go
frewilhelm
frewilhelm reviewed Dec 11, 2025
View reviewed changes
Comment thread bindings/go/oci/repository/repository_test.go Outdated
@Skarlso Skarlso force-pushed the trim-trailing-slash branch from 8ae60b4 to 7125f3d Compare December 11, 2025 15:48
frewilhelm
frewilhelm previously approved these changes Dec 11, 2025
View reviewed changes
@Skarlso
fix: trim trailing slashes from the base-path
ae0a232 
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com>

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
@Skarlso Skarlso force-pushed the trim-trailing-slash branch from 7125f3d to ae0a232 Compare December 11, 2025 16:14
@github-actions github-actions Bot added the size/m Medium label Dec 11, 2025
jakobmoellerdev
jakobmoellerdev approved these changes Dec 11, 2025
View reviewed changes

@jakobmoellerdev jakobmoellerdev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This fix is part of normalisation of a URl and I believe that needs to be part of the runtime URL as @frewilhelm pointed out. approving nevertheless but this needs follow-up imho

@Skarlso

Skarlso commented Dec 11, 2025

Copy link
Copy Markdown
Contributor Author

I'm not using the outcome of that. But I'll explain a bit better later.

@Skarlso

Skarlso commented Dec 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Okay, so, the problem with just doing this in ParseURLAndAllowNoScheme is that I'm not actually using that result in the baseURL, right? Because, that results in the port being cut off if it exists and put into the port section. And also, the path is now cut. I could re-assemble that and recreate the baseURL again in its original format, but that is prone to error in some form or the other.

So what I want is to use the original format. How it was defined originally! Especially if path WAS set, because then the baseURL must remain as it was! So if it was domain:5555/org and subPath is set to repo then it needs to remain domain:5555/org for later the reference parser to work and repo being added correctly.

That said, of course we can try and refactor this entire thing and then construct the original URL back into how the baseurl was defined, but I feel like that's a hassle. :D

And that is why. :)

@frewilhelm frewilhelm merged commit d29be10 into open-component-model:main Dec 12, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@frewilhelm frewilhelm frewilhelm approved these changes

@jakobmoellerdev jakobmoellerdev jakobmoellerdev approved these changes

Assignees

No one assigned

Labels

kind/bugfix Bug size/m Medium size/s Small

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Skarlso @frewilhelm @jakobmoellerdev