Skip to content

feature/plugins: config layout, --plugins flag, and plugin name validation #1403

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 25, 2020
Merged

feature/plugins: config layout, --plugins flag, and plugin name validation #1403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
pkg/cli: use --plugins to determine which plugin.Base's kubebuilder 
should run for a subcommand. This option is global. Refactoring of
getBaseFlags -> parseBaseFlags allows multiple global flags to be
parsed for a cli struct. plugin filtering considers short names,
ex. --plugins="go" rather than --plugins="go.kubebuilder.io/v2.0.0".
WithDefaultPlugins sets the default plugins a CLi should use in case
--plugins or the config 'layout' are not set

pkg/model/config: Config support the 'layout' key which indicates the
plugin that scaffolded a project, by plugin key

pkg/plugin: SplitKey, KeyFor, Key, GetShortName are utility functions
for plugin keys

* pkg/{cli,plugin}: add plugin name validation

* pkg/plugin/{v1,v2}: set layout key in config

 Please enter the commit message for your changes. Lines starting
  • Loading branch information
@estroz
estroz committed Mar 24, 2020
commit f460cc77f4b57e2da9bf216d7f8be217f0d28319
4 changes: 4 additions & 0 deletions cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ func main() {
&pluginv1.Plugin{},
&pluginv2.Plugin{},
),
cli.WithDefaultPlugins(
&pluginv1.Plugin{},
&pluginv2.Plugin{},
),
Copy link
Member

@camilamacedo86 camilamacedo86 Mar 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why we need to:

		cli.WithPlugins(
			&pluginv1.Plugin{},
			&pluginv2.Plugin{},
		),
		cli.WithDefaultPlugins(
			&pluginv1.Plugin{},
			&pluginv2.Plugin{},
		),

What is the diff between plugin and default plugin?
What does the default plugin mean? When the default plugin should or not be used?
Is not it redundant?

Copy link
Contributor Author

@estroz estroz Mar 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Imagine the case where there are multiple plugins per project version available to a user, ex. two different Go plugins go and go-x that support project version 2:

		cli.WithPlugins(
			&pluginv1.Plugin{}, // Supports project version 1
			&pluginv2.Plugin{}, // Supports project version 2
			&gox.Plugin{},		// Supports project version 2
		),
		cli.WithDefaultPlugins(
			&pluginv1.Plugin{},
			&pluginv2.Plugin{},
		),

One of those two should be defaulted to in case the user doesn’t set —plugins on init or they don’t have a layout key in their config (project version 1). However the CLI won’t know which it should default to unless one is explicitly set. See #1403 (comment) for more details on why I chose to set defaults explicitly.

cli.WithExtraCommands(
newEditCmd(),
newUpdateCmd(),
Expand Down
2 changes: 1 addition & 1 deletion designs/extensible-cli-and-scaffolding-plugins-phase-1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ project versions (via `SupportedProjectVersions()`).
Example `PROJECT` file:

```yaml
version: "3-alpha"
version: "2"
Copy link
Contributor

@DirectXMan12 DirectXMan12 Mar 26, 2020

Choose a reason for hiding this comment

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

??

Copy link
Member

@camilamacedo86 camilamacedo86 Mar 27, 2020

Choose a reason for hiding this comment

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

@DirectXMan12
I understand that the version here will still be 2 since we have not broken changes in the layout. However, kb tag will need to be bumped to 3.0.0. Am I right?

Copy link
Contributor Author

@estroz estroz Mar 27, 2020

Choose a reason for hiding this comment

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

There was pushback against upgrading to "3-alpha". Regardless of whether we bump I figured it was better to make the change in a follow-up.

Copy link
Contributor

@DirectXMan12 DirectXMan12 Apr 6, 2020

Choose a reason for hiding this comment

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

that's ok, but I think if we're gonna make a field mandatory that's a change in version.

layout: go/v1.0.0
domain: testproject.org
repo: github.com/test-inc/testproject
Expand Down
7 changes: 1 addition & 6 deletions pkg/cli/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,9 @@ func (c cli) newAPIContext() plugin.Context {
}

func (c cli) bindCreateAPI(ctx plugin.Context, cmd *cobra.Command) {
versionedPlugins, err := c.getVersionedPlugins()
if err != nil {
cmdErr(cmd, err)
return
}
var getter plugin.CreateAPIPluginGetter
var hasGetter bool
for _, p := range versionedPlugins {
for _, p := range c.resolvedPlugins {
tmpGetter, isGetter := p.(plugin.CreateAPIPluginGetter)
if isGetter {
if hasGetter {
Expand Down
Loading