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: improve plugin installation UX #617

Closed

Conversation

duffney
Copy link
Contributor

@duffney duffney commented Apr 4, 2023

Adds an install and remove subcommand to the notation plugin command. The install command takes a plugin binary and copies it to the plugin directory. If the plugin directory doesn't exist, it is created, and the plugin copied. If an exist plugin exists, the version is compared and copied when the current version is less than the version of the plugin binary provided as an argument. A --force flag is available to overwrite this logic and copy the plugin binary regardless of the existing plugin version. The remove command removes the plugin specific directory and its contents.

cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
@yizha1
Copy link
Contributor

yizha1 commented Apr 7, 2023

@priteshbandi @shizhMSFT @FeynmanZhou I suggest we also put this new subcommand install as experimental command since install command takes a plugin binary only, how can users get to know what kind of file should be used. So, it will be good to let users try this command first and provide feedback.

@shizhMSFT shizhMSFT changed the title Feat: Improve plugin installation UX feat: improve plugin installation UX Apr 10, 2023
@@ -36,6 +44,44 @@ Example - List installed Notation plugins:
}
}

func pluginInstallCommand() *cobra.Command {
var force bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make a struct instead of a single variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any other config settings we'd like to add to the struct? If so, it might be worthwhile to look into adding viper for configuration.

cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
@shizhMSFT
Copy link
Contributor

@priteshbandi I'd like to see if notation plugin install from a tar gz file. There is no security risk if we require the file structure in the tar ball to be flatten and only allow regular files. No symlinks and hard links of course.

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2023

Codecov Report

Merging #617 (3eb146c) into main (c9d3997) will decrease coverage by 2.92%.
The diff coverage is 19.01%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #617      +/-   ##
==========================================
- Coverage   63.77%   60.86%   -2.92%     
==========================================
  Files          40       40              
  Lines        2228     2379     +151     
==========================================
+ Hits         1421     1448      +27     
- Misses        686      808     +122     
- Partials      121      123       +2     
Impacted Files Coverage Δ
cmd/notation/plugin.go 35.86% <19.01%> (-56.99%) ⬇️

... and 1 file with indirect coverage changes

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

@duffney
Copy link
Contributor Author

duffney commented Apr 17, 2023

@priteshbandi @shizhMSFT @FeynmanZhou I suggest we also put this new subcommand install as experimental command since install command takes a plugin binary only, how can users get to know what kind of file should be used. So, it will be good to let users try this command first and provide feedback.

Is there anything I need to do to mark it as experimental?

@yizha1
Copy link
Contributor

yizha1 commented Apr 18, 2023

@priteshbandi @shizhMSFT @FeynmanZhou I suggest we also put this new subcommand install as experimental command since install command takes a plugin binary only, how can users get to know what kind of file should be used. So, it will be good to let users try this command first and provide feedback.

Is there anything I need to do to mark it as experimental?

@duffney Sorry. Please ignore my comments for now.

cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
cmd/notation/plugin.go Outdated Show resolved Hide resolved
@duffney
Copy link
Contributor Author

duffney commented Jun 5, 2023

My apologies to everyone for the delay in replying to these comments. I'm actively re-reviewing all the comments and working to resolve them. Thank you for your patience. :)

@duffney duffney requested a review from Two-Hearts as a code owner June 9, 2023 15:17
duffney and others added 6 commits June 9, 2023 10:18
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Co-authored-by: Pritesh Bandi <priteshbandi@gmail.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Co-authored-by: Pritesh Bandi <priteshbandi@gmail.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
duffney and others added 7 commits June 9, 2023 10:18
Co-authored-by: Pritesh Bandi <priteshbandi@gmail.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Co-authored-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Co-authored-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
@duffney duffney force-pushed the feat-improve-plugin-installation-ux branch from b56f772 to 7f9d96f Compare June 9, 2023 15:18
Signed-off-by: Josh Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
Signed-off-by: Joshua Duffney <jduffney@microsoft.com>
@duffney
Copy link
Contributor Author

duffney commented Jun 13, 2023

@priteshbandi I'd like to see if notation plugin install from a tar gz file. There is no security risk if we require the file structure in the tar ball to be flatten and only allow regular files. No symlinks and hard links of course.

I agree. Users will expect the subcommand to handle archives because that's how they're downloaded. If it's not added to this PR, it might be worth adding error handling for people who pass in a .zip or .tar.gz to throw and error saying it needs to be extracted.

@yizha1
Copy link
Contributor

yizha1 commented Jan 11, 2024

Closed as plugin installation was covered by a different PR #827

@yizha1 yizha1 closed this Jan 11, 2024
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.

None yet

8 participants