Skip to content
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

Remove misleading "must" in ref.name requirements #1196

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

tianon
Copy link
Member

@tianon tianon commented Aug 8, 2024

See b692dee (#695) for where this was originally added (making clear this was the original intent).

IMO it slightly conflicts with the bullet point above it being simply SHOULD, but it has been part of the spec since v1.0 (and in a disagreement between MUST and SHOULD, the stronger constraint wins and that's MUST).

Updated: #1196 (review)

@tianon
Copy link
Member Author

tianon commented Aug 8, 2024

Perhaps this should be SHOULD instead to match the earlier requirement? I'm happy to update.

@tianon
Copy link
Member Author

tianon commented Aug 8, 2024

(however, to be explicit, I do think using "must" here is a bug and we should fix it to either MUST or SHOULD so the intent is 100% clear and no longer ambiguous -- MUST is more consistent with the language used, but SHOULD is more consistent with the rest of the annotation's stated limitations)

Copy link
Contributor

@sudo-bmitch sudo-bmitch left a comment

Choose a reason for hiding this comment

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

This makes me really want to define a reference somewhere in OCI, and then have this as a pointer to that definition. Given that the previous line says "SHOULD", I think converting this to a hard MUST is incompatible with the previous line and could break anyone that is treating themselves as an exception to the SHOULD.

I'd lean towards removing "must" completely, and just say "A valid reference matches the following grammar". That would straddle the "here's what you should do" with the hint of "don't be surprised if there are implementations doing invalid stuff". I'd also accept changing this from "must" to "SHOULD".

@tianon tianon changed the title Fix capitalization of MUST in ref.name requirements Remove misleading "must" in ref.name requirements Aug 19, 2024
@tianon
Copy link
Member Author

tianon commented Aug 19, 2024

Sure, no strong argument here -- updated!

@tianon
Copy link
Member Author

tianon commented Aug 19, 2024

(CI failure is unrelated 🙃)

@hassanrawan887

This comment was marked as spam.

Copy link
Contributor

@sudo-bmitch sudo-bmitch left a comment

Choose a reason for hiding this comment

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

LGTM

@sudo-bmitch sudo-bmitch merged commit 3c3d717 into opencontainers:main Aug 22, 2024
1 of 4 checks passed
@tianon tianon deleted the ref.name-MUST branch August 22, 2024 22:00
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.

4 participants