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

feat: blob sign command #1128

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Conversation

Two-Hearts
Copy link
Contributor

@Two-Hearts Two-Hearts commented Dec 27, 2024

This PR adds notation blob sign command with E2E tests.

It's an implementation of spec: https://github.com/notaryproject/notation/blob/main/specs/commandline/blob.md#notation-blob-sign

Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Copy link

codecov bot commented Dec 27, 2024

Codecov Report

Attention: Patch coverage is 93.17073% with 14 lines in your changes missing coverage. Please review.

Project coverage is 72.52%. Comparing base (fd00032) to head (d7e881c).

Files with missing lines Patch % Lines
cmd/notation/blob/sign.go 91.46% 11 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1128      +/-   ##
==========================================
+ Coverage   70.79%   72.52%   +1.72%     
==========================================
  Files          48       50       +2     
  Lines        2945     3126     +181     
==========================================
+ Hits         2085     2267     +182     
+ Misses        668      667       -1     
  Partials      192      192              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
specs/commandline/blob.md Outdated Show resolved Hide resolved
specs/commandline/blob.md Outdated Show resolved Hide resolved
internal/cmd/signer.go Outdated Show resolved Hide resolved
@Two-Hearts Two-Hearts requested a review from shizhMSFT December 31, 2024 03:21
@Two-Hearts Two-Hearts requested a review from JeyJeyGao January 8, 2025 06:46
JeyJeyGao
JeyJeyGao previously approved these changes Jan 8, 2025
Copy link
Contributor

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion.

cmd/notation/blob/sign.go Show resolved Hide resolved
Signed-off-by: Patrick Zheng <[email protected]>
go.mod Show resolved Hide resolved
@Two-Hearts Two-Hearts requested a review from JeyJeyGao January 8, 2025 07:51
JeyJeyGao
JeyJeyGao previously approved these changes Jan 8, 2025
Copy link
Contributor

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
@Two-Hearts Two-Hearts requested a review from JeyJeyGao January 10, 2025 03:02
@JeyJeyGao
Copy link
Contributor

JeyJeyGao commented Jan 10, 2025

image
There 2 issues:

  1. [FAIL] notation blob sign [It] with no permission to write the signature file: this test is failing randomly.
  2. some files are generated during test in the repo. We should generate them in temp folders and delete them after testing.
    test/e2e/suite/command/blob/blobFile.cose.sig
    test/e2e/suite/command/blob/blobFile.jws.sig
    test/e2e/testdata/blob/blobFile.jws.sig
    @Two-Hearts

shizhMSFT
shizhMSFT previously approved these changes Jan 10, 2025
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@Two-Hearts
Copy link
Contributor Author

Two-Hearts commented Jan 10, 2025

  1. some files are generated during test in the repo. We should generate them in temp folders and delete them after testing.
    test/e2e/suite/command/blob/blobFile.cose.sig
    test/e2e/suite/command/blob/blobFile.jws.sig
    test/e2e/testdata/blob/blobFile.jws.sig

@JeyJeyGao These files will not be generated in the repo when the e2e test is run on a GitHub runner, for example, in a PR build. If I choose to run make e2e at local as a developer, then I expect to see the resulted signatures in an E2E scenario. Just like in a real case, when I sign with Notation, I expect to see the signature right?

@JeyJeyGao
Copy link
Contributor

  1. some files are generated during test in the repo. We should generate them in temp folders and delete them after testing.
    test/e2e/suite/command/blob/blobFile.cose.sig
    test/e2e/suite/command/blob/blobFile.jws.sig
    test/e2e/testdata/blob/blobFile.jws.sig

@JeyJeyGao These files will not be generated in the repo when the e2e test is run on a GitHub runner, for example, in a PR build. If I choose to run make e2e at local as a developer, then I expect to see the resulted signatures in an E2E scenario. Just like in a real case, when I sign with Notation, I expect to see the signature right?

Running make e2e locally is an expected step for development. If we create some temporary files without deleting them, we have to manually delete them to maintain a clean git status, which is inconvenient for developers.

Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
@Two-Hearts
Copy link
Contributor Author

Two-Hearts commented Jan 10, 2025

Running make e2e locally is an expected step for development. If we create some temporary files without deleting them, we have to manually delete them to maintain a clean git status, which is inconvenient for developers.

Makes sense. Updated all related test cases.

@Two-Hearts Two-Hearts requested a review from shizhMSFT January 10, 2025 06:01
test/e2e/suite/command/blob/sign.go Outdated Show resolved Hide resolved
test/e2e/suite/command/blob/sign.go Outdated Show resolved Hide resolved
test/e2e/suite/command/blob/sign.go Outdated Show resolved Hide resolved
test/e2e/suite/command/blob/sign.go Outdated Show resolved Hide resolved
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
@Two-Hearts Two-Hearts requested a review from JeyJeyGao January 10, 2025 06:21
Signed-off-by: Patrick Zheng <[email protected]>
Copy link
Contributor

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants