Prevent push cmd failure in 3.18 by handling version tag resolution in ORAS memory store #30894
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- The newReference() function transforms version tags by replacing + with _ for OCI compatibility - But the code was using the original ref (with +) for TagBytes() - Then it tries to find the tagged reference using parsedRef.String() (with _) - This mismatch causes the Resolve method to fail with "not found" - By using parsedRef.String() consistently in both places, the references will match and the lookup will succeed. I extracted the TagBytes function to improve testability. Push() includes several external calls that are hard to mock, so isolating this logic makes testing more manageable. Close: helm#30881 Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
3 tasks
TerryHowe
suggested changes
May 22, 2025
| @@ -685,19 +685,9 @@ func (c *Client) Push(data []byte, ref string, options ...PushOption) (*PushResu | |||
| }) | |||
|
|
|||
| ociAnnotations := generateOCIAnnotations(meta, operation.creationTime) | |||
There was a problem hiding this comment.
I think the parsedRef, err := newReference(ref) above should be removed
There was a problem hiding this comment.
Not sure I get it. The parsedRef is used here ? 🤔
There was a problem hiding this comment.
Line 611. It won't let me comment up there
There was a problem hiding this comment.
Is that what you had in mind? f939f61
TerryHowe
reviewed
May 22, 2025
| } | ||
|
|
||
| return oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest, | ||
| manifestData, parsedRef.String()) |
There was a problem hiding this comment.
This is the only actual change here I think, using parsedRef.String() instead of ref although the tests are nice.
There was a problem hiding this comment.
Yes, exactly. I should have specified it.
There was a problem hiding this comment.
Simplified it's this.
- oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest, manifestData, ref)
+ oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest, manifestData, parsedRef.String())There was a problem hiding this comment.
Yeh, this PR could in theory be just that one line change the way I read it.
Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
2c01f66 to
f939f61
Compare
TerryHowe
approved these changes
May 23, 2025
| @@ -685,19 +685,9 @@ func (c *Client) Push(data []byte, ref string, options ...PushOption) (*PushResu | |||
| }) | |||
|
|
|||
| ociAnnotations := generateOCIAnnotations(meta, operation.creationTime) | |||
mattfarina
approved these changes
May 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Fix: #30881
Initial work for
dev-v3in #30889 (comment) but as requested by @gjenkins8, we should backport after on v3. :)The issue is:
newReference()function transforms version tags by replacing + with _ for OCI compatibility (as we can see in the output message)TagBytes()parsedRef.String()(with _)Resolvemethod to fail with "not found"parsedRef.String()consistently in both places, the references will match and the lookup will succeed.I extracted the
TagBytesfunction to improve testability.Push()includes several external calls that are hard to mock, so isolating this logic makes testing more manageable.The patch could be summarized as:
The initial regression is something that was missed in 5a7046b#diff-acbc1e889c3e91003a88f18e21c3073b64937801b20f663fcf2601540b9e3cdaR709
Before :
After :
Special notes for your reviewer:
If applicable:
docs neededlabel should be applied if so)