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

doc(spec): add subcommands to Notation plugin spec #555

Merged
merged 9 commits into from
Mar 29, 2023

Conversation

duffney
Copy link
Contributor

@duffney duffney commented Feb 15, 2023

Spec changes to improve the UX for plugin installation and removal using the NotationCLI.

specs/commandline/plugin.md Outdated Show resolved Hide resolved
specs/commandline/plugin.md Outdated Show resolved Hide resolved
specs/commandline/plugin.md Outdated Show resolved Hide resolved
@shizhMSFT shizhMSFT changed the title Add subcommands to Notation plugin spec Issue #549 doc(spec): add subcommands to Notation plugin spec Feb 16, 2023
@shizhMSFT
Copy link
Contributor

Linking the parent issue #549

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2023

Codecov Report

Merging #555 (378e92c) into main (54b42cb) will decrease coverage by 1.98%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #555      +/-   ##
==========================================
- Coverage   36.34%   34.36%   -1.98%     
==========================================
  Files          30       32       +2     
  Lines        1607     1848     +241     
==========================================
+ Hits          584      635      +51     
- Misses       1002     1192     +190     
  Partials       21       21              

see 4 files with indirect coverage changes

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

@yizha1
Copy link
Contributor

yizha1 commented Feb 20, 2023

@duffney Could you fix the DCO issue, see below check results.

duffney and others added 3 commits February 22, 2023 18:58
Co-authored-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: GitHub <noreply@github.com>
Co-authored-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
specs/commandline/plugin.md Outdated Show resolved Hide resolved
specs/commandline/plugin.md Outdated Show resolved Hide resolved
@yizha1
Copy link
Contributor

yizha1 commented Mar 7, 2023

@duffney Could you resolve the comments, so that we can move on?

Co-authored-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: GitHub <noreply@github.com>
@duffney duffney force-pushed the gh-549/notation-plugin-spec branch from 7d4f6a8 to 626442c Compare March 7, 2023 16:15
@duffney
Copy link
Contributor Author

duffney commented Mar 7, 2023

@duffney Could you resolve the comments, so that we can move on?

I resolved the open comments, but there are a few unanswered questions around the --plugin-name argument. Mainly around what to do when the option isn't provided and how notation would handle plugin-names that doesn't follow the naming standard.

@yizha1
Copy link
Contributor

yizha1 commented Mar 8, 2023

@duffney Could you resolve the comments, so that we can move on?

I resolved the open comments, but there are a few unanswered questions around the --plugin-name argument. Mainly around what to do when the option isn't provided and how notation would handle plugin-names that doesn't follow the naming standard.

@duffney I think it is connected to the previous comment from Shiwei, see here #555 (comment). The --plugin-name is optional and used to confirm it is the plugin that users want to install. That is to say, Users cannot use a different name for the plugin. I would suggest the explanation text for the --plugin-name would be used for confirming it is the right plugin, not a new name that users can use.

Here is a copy with additional comments added from me. IMO, Case-1 is the most common case.

Case-1: Name is retrieved from the plugin metadata, but need users to confirm it is the right one.
$ notation plugin install notation-example-foo.tgz
The NEW plugin example-foo will be installed.
Do you want to continue? [y/N] y
The plugin example-foo is successfully installed.

Case-2: users specify the name which is the same as the name from plugin metadata, then it will be installed directly
$ notation plugin install --name example-bar notation-example-bar.tgz
The plugin example-bar is successfully installed.

Case-3: users specify the name, which is not the same from plugin metadata
$ notation plugin install --name example-hello notation-example-foo.tgz
Error: notation-example-foo.tgz: example-hello not found

specs/commandline/plugin.md Outdated Show resolved Hide resolved
@yizha1
Copy link
Contributor

yizha1 commented Mar 10, 2023

@duffney Could you resolve the comments? The RC-4 release (Previous RC-3) is planned by end of this Month. Hope we can get the spec merged next week, then we can start the implementation.

@duffney
Copy link
Contributor Author

duffney commented Mar 20, 2023

@duffney Could you resolve the comments, so that we can move on?

I resolved the open comments, but there are a few unanswered questions around the --plugin-name argument. Mainly around what to do when the option isn't provided and how notation would handle plugin-names that doesn't follow the naming standard.

@duffney I think it is connected to the previous comment from Shiwei, see here #555 (comment). The --plugin-name is optional and used to confirm it is the plugin that users want to install. That is to say, Users cannot use a different name for the plugin. I would suggest the explanation text for the --plugin-name would be used for confirming it is the right plugin, not a new name that users can use.

Here is a copy with additional comments added from me. IMO, Case-1 is the most common case.

Case-1: Name is retrieved from the plugin metadata, but need users to confirm it is the right one. $ notation plugin install notation-example-foo.tgz The NEW plugin example-foo will be installed. Do you want to continue? [y/N] y The plugin example-foo is successfully installed.

Case-2: users specify the name which is the same as the name from plugin metadata, then it will be installed directly $ notation plugin install --name example-bar notation-example-bar.tgz The plugin example-bar is successfully installed.

Case-3: users specify the name, which is not the same from plugin metadata $ notation plugin install --name example-hello notation-example-foo.tgz Error: notation-example-foo.tgz: example-hello not found

@yizha1 having the name option confirm the correct plugin name makes perfect sense to me, thanks for the clarification. :)

shizhMSFT
shizhMSFT previously approved these changes Mar 20, 2023
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

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

JeyJeyGao
JeyJeyGao previously approved these changes Mar 21, 2023
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: Josh Duffney <jduffney@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

Signed-off-by: Josh Duffney <jduffney@microsoft.com>
priteshbandi
priteshbandi previously approved these changes Mar 28, 2023
Copy link
Contributor

@priteshbandi priteshbandi 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 nitpick that Shiwei called out.

Signed-off-by: GitHub <noreply@github.com>
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

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

@yizha1 yizha1 merged commit 56945b3 into notaryproject:main Mar 29, 2023
duffney added a commit to duffney/notation that referenced this pull request Mar 30, 2023
Spec changes to improve the UX for plugin installation and removal using
the NotationCLI.

---------

Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: GitHub <noreply@github.com>
Co-authored-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
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.

7 participants