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

accepts attachment-tag-prefix for cosign copy #3014

Merged

Conversation

mritunjaysharma394
Copy link
Contributor

Summary

This PR was created in an attempt that closes #2962 where in the case of cosign copy, attachment-tag-prefix was ignored.

Release Note

Fixed cosign copy ignoring attachment-tag-prefix flag

Documentation

Nope, I guess.

@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

Merging #3014 (7ea5d75) into main (6236366) will increase coverage by 0.21%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #3014      +/-   ##
==========================================
+ Coverage   30.23%   30.45%   +0.21%     
==========================================
  Files         151      152       +1     
  Lines        9478     9584     +106     
==========================================
+ Hits         2866     2919      +53     
- Misses       6167     6208      +41     
- Partials      445      457      +12     
Impacted Files Coverage Δ
cmd/cosign/cli/copy/copy.go 50.00% <66.66%> (ø)

Signed-off-by: Mritunjay <mritunjaysharma394@gmail.com>
@mritunjaysharma394 mritunjaysharma394 force-pushed the cosign-copy-attachment-tag-prefix branch from 78f875e to ff25b99 Compare May 29, 2023 18:06
@hectorj2f
Copy link
Contributor

@mritunjaysharma394 Thanks for the PR.

Would you mind adding a test to validate the problem reported in the issue?

Signed-off-by: Mritunjay <mritunjaysharma394@gmail.com>
@mritunjaysharma394
Copy link
Contributor Author

Hi @hectorj2f, thanks for the quick review, I was a little unsure of how to get tag/hash/digest for the test code to verify if the tag-prefix worked the way we want or not since in test-code, I assume we arent talking to the remote registry. However, I still tried to write one, feel free to review and help me with improvements, here's the debug console output which shows it works, thanks a lot:
image

@mritunjaysharma394
Copy link
Contributor Author

Also @hectorj2f, I just checked it with the repro that @znewman01 mentioned while creating the issue, I followed the exact steps and it seems to be working now:
image

@hectorj2f hectorj2f requested a review from znewman01 May 30, 2023 19:58
znewman01
znewman01 previously approved these changes May 31, 2023
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

LGTM except the one question I had

cmd/cosign/cli/copy/copy.go Outdated Show resolved Hide resolved
Signed-off-by: Mritunjay <mritunjaysharma394@gmail.com>
@znewman01 znewman01 enabled auto-merge (squash) May 31, 2023 14:08
@znewman01
Copy link
Contributor

LGTM, thank you so much!

@znewman01 znewman01 merged commit c17a36a into sigstore:main May 31, 2023
@github-actions github-actions bot added this to the v1.14.0 milestone May 31, 2023
@thangarajav
Copy link

Thanks for fixing the issue, When it will be available for me to download.

@znewman01
Copy link
Contributor

Should have a new release Real Soon Now (TM): sigstore/sigstore#1166 (comment)

:)

@@ -102,6 +109,7 @@ func CopyCmd(ctx context.Context, regOpts options.RegistryOptions, srcImg, dstIm
// Copy the entity itself.
g.Go(func() error {
dst := dstRepoRef.Tag(srcDigest.Identifier())
dst = dst.Tag(fmt.Sprint(regOpts.RefOpts.TagPrefix, h.Algorithm, "-", h.Hex))
Copy link
Member

Choose a reason for hiding this comment

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

This means we will never copy the tag itself 🤔

cc @haydentherapper

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I'm not sure what dstRepoRef.Tag(srcDigest.Identifier()) is even meant to do either 😅

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.

cosign copy ignores --attachment-tag-prefix
5 participants