Skip to content

spec: update cli sign spec for tag to digest translation#439

Merged
priteshbandi merged 29 commits intonotaryproject:mainfrom
yizha1:sign_ux
Dec 2, 2022
Merged

spec: update cli sign spec for tag to digest translation#439
priteshbandi merged 29 commits intonotaryproject:mainfrom
yizha1:sign_ux

Conversation

@yizha1
Copy link
Contributor

@yizha1 yizha1 commented Nov 8, 2022

This PR is mainly to improve the output message of notation sign command for tag to digest translation.

@yizha1 yizha1 added the spec Specifications to define the product requirements label Nov 8, 2022
@yizha1 yizha1 self-assigned this Nov 8, 2022
@yizha1 yizha1 linked an issue Nov 8, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2022

Codecov Report

Merging #439 (5b9e582) into main (40973f9) will decrease coverage by 0.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #439      +/-   ##
==========================================
- Coverage   32.82%   32.48%   -0.35%     
==========================================
  Files          25       25              
  Lines        1237     1250      +13     
==========================================
  Hits          406      406              
- Misses        819      832      +13     
  Partials       12       12              
Impacted Files Coverage Δ
cmd/notation/verify.go 22.07% <0.00%> (-2.93%) ⬇️
cmd/notation/cert/generateTest.go 16.66% <0.00%> (-1.26%) ⬇️
cmd/notation/list.go 23.28% <0.00%> (-0.66%) ⬇️
cmd/notation/plugin.go 0.00% <0.00%> (ø)
cmd/notation/registry.go 0.00% <0.00%> (ø)
cmd/notation/key.go 24.75% <0.00%> (+0.24%) ⬆️
cmd/notation/sign.go 45.45% <0.00%> (+8.22%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@vaninrao10
Copy link
Contributor

I think the verbiage should be changed to - [ Sign an OCI artifact stored in a registry and use ”--expiry”(expiration) parameter to define the expiration duration for the signature in days, for example 1day = 24 hours. ]

Signed-off-by: Yi Zha <yizha1@microsoft.com>
@yizha1 yizha1 requested a review from priteshbandi November 24, 2022 03:34
Copy link
Member

@FeynmanZhou FeynmanZhou left a comment

Choose a reason for hiding this comment

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

Overall looks good. Only two concerns left above

Signed-off-by: Yi Zha <yizha1@microsoft.com>
Copy link
Contributor

@vaninrao10 vaninrao10 left a comment

Choose a reason for hiding this comment

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

LGTM

@iamsamirzon
Copy link
Contributor

LGTM

Comment on lines 18 to 19
Warning: Always sign the artifact using digest(`@sha256:...`) rather than a tag(`:v1`) because tags are mutable and a tag reference can point to a different artifact than the one signed.
Resolved artifact tag '<tag>' to digest '<digest>' before signing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: These outputs will be in the form of logs.

# Or change the default signing key to an existing signing key
notation key update --default <key_name>
# Prerequisites:
# - A signing plugin is installed. See plugin documentation (https://github.com/notaryproject/notaryproject/blob/main/specs/plugin-extensibility.md) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the main branch, should we use a tag version for the URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we update it later once we have cut a version for notaryproject?

Upon successful signing, the generated signature is pushed to the registry and associated with the signed OCI artifact. The output message is printed out as following:

```text
Successfully signed <registry>/<repository>@<digest>.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems not consistent with notation verify that the sentence has a dot at the end or not.


```console
$ notation sign localhost:5000/net-monitor:v1
Warning: Always sign the artifact using digest(`@sha256:...`) rather than a tag(`:v1`) because tags are mutable and a tag reference can point to a different artifact than the one signed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Warning: Always sign the artifact using digest(`@sha256:...`) rather than a tag(`:v1`) because tags are mutable and a tag reference can point to a different artifact than the one signed.
Warning: Always sign the artifact using digest(`@sha256:...`) rather than a tag(`:<tag>`) because tags are mutable and a tag reference can point to a different artifact than the one signed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would keep line 115 as it is, but change the text in line 18 from :v1 to :<tag>. What do you think?

Signed-off-by: Yi Zha <yizha1@microsoft.com>
@yizha1 yizha1 requested a review from shizhMSFT December 1, 2022 08:56
Signed-off-by: Yi Zha <yizha1@microsoft.com>
Copy link
Contributor

@patrickzheng200 patrickzheng200 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

@yizha1 yizha1 dismissed toddysm’s stale review December 2, 2022 04:26

Discussed with Toddy, and looks good to him

@priteshbandi priteshbandi merged commit 1b186ad into notaryproject:main Dec 2, 2022
7h3-3mp7y-m4n pushed a commit to 7h3-3mp7y-m4n/notation that referenced this pull request Mar 29, 2025
…ct#439)

This PR is mainly to improve the output message of `notation sign`
command for tag to digest translation.

Signed-off-by: Yi Zha <zhayi@outlook.com>
Signed-off-by: Yi Zha <yizha1@microsoft.com>
FeynmanZhou pushed a commit to FeynmanZhou/notation that referenced this pull request May 15, 2025
…ct#439)

This PR is mainly to improve the output message of `notation sign`
command for tag to digest translation.

Signed-off-by: Yi Zha <zhayi@outlook.com>
Signed-off-by: Yi Zha <yizha1@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spec Specifications to define the product requirements

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

UX: Improve the output for a successful signing

9 participants

Comments