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

Allow plugins to be specified via a URL #989

Merged
merged 1 commit into from
Nov 12, 2019
Merged

Allow plugins to be specified via a URL #989

merged 1 commit into from
Nov 12, 2019

Conversation

johnSchnake
Copy link
Contributor

What this PR does / why we need it:
We currently allow loading plugins from the local file system
but for sharing of plugins it makes sense to allow users to
specify a URL.

Which issue(s) this PR fixes
Fixes #986

Special notes for your reviewer:

Release note:

The `--plugin` flag now supports URLs in addition to paths to local files or directories.

@johnSchnake johnSchnake requested a review from zubron November 10, 2019 15:59
@johnSchnake
Copy link
Contributor Author

Not sure why but the 2 circleci checks are hung; they seemed to have pass but just not get reported to Github


newPlugin, err := loadManifest(b)
if err != nil {
return errors.Wrapf(err, "failed to load plugin file %q", filepath)
return errors.Wrap(err, "failed to load plugin")
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor usability nit: If the user specifies multiple plugins and one fails to decode, it seems like it won't be as easy to determine which one had the issue. Instead, could you return the unwrapped error here and wrap where this function is called in loadSinglePluginFromURL and loadSinglePluginFromFile? That way both of those functions could include the location information of the plugin in the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of just returning the value of loadSinglePlugin I wrapped that value with the context necessary to describe the URL or file. 👍

Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Just a small suggestion regarding error messages, but otherwise this looks good! 👍

We currently allow loading plugins from the local file system
but for sharing of plugins it makes sense to allow users to
specify a URL.

Fixes #986

Signed-off-by: John Schnake <[email protected]>
@johnSchnake johnSchnake merged commit 73cdb50 into vmware-tanzu:master Nov 12, 2019
@johnSchnake johnSchnake deleted the remoteLoadPlugin branch November 12, 2019 12:38
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.

Load plugins from a given URL
2 participants