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

estroz · 2020-03-03T17:12:07Z

Description of Change:

pkg/cli: --plugins allows a binary invoker to select which plugins known to a CLI to run; this flag can be set with a shortened (non-fully-qualified) name. Either a plugin name passed to --plugins or the name set in an initialized project's config layout, in order of precedence, is resolved to a known plugin.
pkg/model/config: add project file layout key, containing the Init plugin key that scaffolded a project
pkg/{cli,plugin}: add plugin name validation
pkg/plugin/v2: set layout key in config

Motivation for Change: finishes up phase 1 of plugin implementation. The layout key determines which plugin (name and version) initialized a project, useful for a plugin implementation to deduce which post-init plugins should be called. --plugins/layout, in combination with a default plugin, are necessary to select which plugin to use if there are multiple viable plugins per project version.

TODO:

instead of getting first *Getter type, infer by config layout or default (in the case of Init)
unit tests in pkg/cli

/cc @Adirio @hasbro17 @camilamacedo86
/kind feature

Adirio

I don't think we need to move to scaffold v3 (or v3-alpha) to implement the plugins system. We definetely will need to move to kubebuilder v3.y.z. This will influence some of the following comments.

In order to keep it in v2 we need to make it backwards compatible. Providing a default value for layout in case the project file doesn't have it has already be done for the version field, that defaults to v1 at read time as v1 projects didn't have that field. In this case it will only be needed to set the layout field to the default go plugin when reading a config file that has version: "2" but no layout field.

Adirio · 2020-03-04T07:56:12Z

cmd/edit.go


 func (o *editOptions) Validate(c *config.Config) error {
-	if !c.IsV2() {
+	if c.IsV1() {


(Remember I consider we should not use v3-alpha)

Right now they are functionally equivalents but they have different semantics. !c.IsV2() makes no assumption about how following versions will work while c.IsV1() makes the sumption that following versions will implement the multi-group flag.

This change is more for convenience, but I'll explicitly check v2 and v3 if we decide to stick with v3.

Ditto https://github.com/kubernetes-sigs/kubebuilder/pull/1403/files#r387589683
I do not think that we should add V3 as well.

Adirio · 2020-03-04T07:57:13Z

internal/config/config.go


 	// Default version if flag not provided
-	DefaultVersion = config.Version2
+	DefaultVersion = config.Version3


(Remember I consider we should not use v3-alpha)

Ditto https://github.com/kubernetes-sigs/kubebuilder/pull/1403/files#r387589683
I do not think that we should add V3 as well.

internal/config/config.go

pkg/cli/api.go

Adirio · 2020-03-04T08:01:50Z

pkg/cli/cli.go

+		pluginsFromOptions:    map[string][]plugin.Base{},
+		cliPluginKeys:         map[string]string{},


I think that linters prefer the following:

Suggested change

pluginsFromOptions: map[string][]plugin.Base{},

cliPluginKeys: map[string]string{},

pluginsFromOptions: make(map[string][]plugin.Base),

cliPluginKeys: make(map[string]string),

Adirio · 2020-03-04T08:53:47Z

pkg/plugin/v2/init.go

+	// Only V3 projects get a layout.
+	if p.config.IsV3() {
+		// Init plugins set the config layout.
+		p.config.Layout = plugin.Key(pluginName, pluginVersion)
+	}


(Remember I consider we should not use v3-alpha)

Ditto https://github.com/kubernetes-sigs/kubebuilder/pull/1403/files#r387589683
I do not think that we should add V3 as well.

pkg/plugin/plugin.go

Adirio · 2020-03-04T09:03:19Z

pkg/plugin/v2/plugin.go

+	pluginVersion = "v2.0.0"
+)
+
+var supportedProjectVersions = []string{"2", "3-alpha"}


(Remember I consider we should not use v3-alpha)

pkg/cli/cli.go

estroz · 2020-03-04T19:35:52Z

I'm fine with keeping config version 2, although the layout key represents a breaking change to a config's structure. This should reasonably be handled by kubebuilder version though.

@DirectXMan12 @mengqiy @joelanford thoughts on changing config version to 3-alpha vs going ahead with 2?

Adirio

Some additional questions and thoughts

pkg/cli/cli.go

camilamacedo86

It is not againt the master so:

/lgtm
/approve

We will come back to re-check all plugin impl in the PR against master which will be easier to verify all.

estroz · 2020-03-23T16:16:48Z

/assign @mengqiy

camilamacedo86 · 2020-03-23T16:26:09Z

/approve

camilamacedo86 · 2020-03-24T07:38:53Z

/approve

k8s-ci-robot · 2020-03-24T07:39:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, estroz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

camilamacedo86 · 2020-03-24T16:44:52Z

Hi @estroz,

It is not passing now, after the rebase, in the testdata check. You need run make generate locally to update the testdata dir with your changes.

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

camilamacedo86 · 2020-03-25T00:44:37Z

Hi @estroz,

The GH did not get the Travis result. If you close and re-open this one the tests will be re-trigged.

camilamacedo86

As we spoke:

/lgtm

Since it is to merge in the kubernetes-sigs:feature/plugins-part-2-electric-boogaloo and not master.
And then we can check the tests in a follow-up pr.

DirectXMan12 · 2020-03-26T19:02:59Z

designs/extensible-cli-and-scaffolding-plugins-phase-1.md


 ```yaml
-version: "3-alpha"
+version: "2"


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

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.

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

DirectXMan12 · 2020-03-26T19:09:48Z

pkg/cli/cli.go

 func (c *cli) initialize() error {
+	// Initialize cli with globally-relevant flags or flags that determine
+	// certain plugin type's configuration.
+	if err := c.parseBaseFlags(); err != nil {


why do we need this globally? isn't this just needed on the init invocation?

The CLI needs to pre-parse the --plugins global flag for each command, so I figured wrapping all flag pre-parsing in one method made sense. The cli.doGenericHelp flag is set by cli.parseBaseFlags so init can still print generic help if --project-version is not set.

The CLI needs to pre-parse the --plugins global flag for each command,

wait, does it? I thought we figured out that it didn't...

Summary of discussion: having a global --plugins flag is not in scope for phase 1 since the type of plugin used in init solely determines what plugin is used for subsequent commands (the layout config key). A global --plugins flag will be discussed in phase 2: #1378.

DirectXMan12 · 2020-03-26T19:11:24Z

pkg/cli/cli.go

+	case len(c.cliPluginKeys) != 0:
+		// Filter plugins by keys passed in CLI.
+		filterKeys = c.cliPluginKeys
+	case c.configured && !projectConfig.IsV1():


doesn't this break current v2 projects?

It will, and probably a good reason to upgrade to project version "3-alpha".

DirectXMan12 · 2020-03-26T19:12:46Z

pkg/cli/cli.go

+		if err != nil {
+			return err
+		}
+	}


what happens if filterKeys is not set here? (i.e. if there's no plugins set for options)?

(if we check elsewhere, add a comment here)

(it's also maybe a good indication that we're failing at the principle of "parse instead of validate")

cli.resolvedPlugins defaults to the default plugins for a project version if filterKeys is empty. Default plugins are ensured to exist by validating them here. Does this still fail parse instead of validate")? I'll add a comment describing this scenario if not.

Looks like the case when c.configured && projectConfig.IsV1() == true is not validated however. Making a follow-up now.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2020

k8s-ci-robot requested review from DirectXMan12 and mengqiy March 3, 2020 17:12

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2020

estroz force-pushed the feature/plugins-config-layout branch from 7b3628b to 98b7b5f Compare March 4, 2020 07:51

estroz changed the title ~~plugins: config layout and plugin name validation~~ [wip] plugins: config layout and plugin name validation Mar 4, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2020

Adirio reviewed Mar 4, 2020

View reviewed changes

camilamacedo86 reviewed Mar 4, 2020

View reviewed changes

pkg/cli/cli.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Mar 4, 2020

View reviewed changes

pkg/cli/cli.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Mar 4, 2020

View reviewed changes

pkg/cli/cli.go Outdated Show resolved Hide resolved

kubernetes-sigs deleted a comment from Adirio Mar 4, 2020

Adirio reviewed Mar 5, 2020

View reviewed changes

pkg/cli/cli.go Outdated Show resolved Hide resolved

pkg/cli/cli.go Outdated Show resolved Hide resolved

pkg/cli/cli.go Outdated Show resolved Hide resolved

estroz force-pushed the feature/plugins-config-layout branch from f7d3815 to 75e1ce8 Compare March 5, 2020 19:45

estroz force-pushed the feature/plugins-config-layout branch 4 times, most recently from 8a654d2 to 23415cf Compare March 20, 2020 21:52

estroz changed the title ~~[wip] plugins: config layout and plugin name validation~~ feature/plugins: config layout, --plugins flag, and plugin name validation Mar 20, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2020

estroz changed the title ~~feature/plugins: config layout, --plugins flag, and plugin name validation~~ [wip] feature/plugins: config layout, --plugins flag, and plugin name validation Mar 20, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2020

estroz force-pushed the feature/plugins-config-layout branch 3 times, most recently from 234e8ad to 1151b04 Compare March 21, 2020 06:00

estroz changed the title ~~[wip] feature/plugins: config layout, --plugins flag, and plugin name validation~~ feature/plugins: config layout, --plugins flag, and plugin name validation Mar 21, 2020

Adirio approved these changes Mar 23, 2020

View reviewed changes

camilamacedo86 approved these changes Mar 23, 2020

View reviewed changes

k8s-ci-robot assigned camilamacedo86 Mar 23, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2020

k8s-ci-robot assigned mengqiy Mar 23, 2020

estroz requested a review from camilamacedo86 March 24, 2020 03:14

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2020

estroz force-pushed the feature/plugins-config-layout branch from da1069b to 2a165d8 Compare March 24, 2020 16:23

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 24, 2020

estroz force-pushed the feature/plugins-config-layout branch from 2a165d8 to 6997218 Compare March 24, 2020 16:35

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 24, 2020

estroz force-pushed the feature/plugins-config-layout branch from 6997218 to f460cc7 Compare March 24, 2020 16:56

estroz closed this Mar 25, 2020

estroz reopened this Mar 25, 2020

camilamacedo86 reviewed Mar 25, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 25, 2020

k8s-ci-robot merged commit 505e23d into kubernetes-sigs:feature/plugins-part-2-electric-boogaloo Mar 25, 2020

estroz deleted the feature/plugins-config-layout branch March 25, 2020 01:57

DirectXMan12 reviewed Mar 26, 2020

View reviewed changes

estroz mentioned this pull request Apr 8, 2020

plugins: move --plugins from global to init-only #1465

Merged

		pluginsFromOptions: map[string][]plugin.Base{},
		cliPluginKeys: map[string]string{},

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

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

Uh oh!

Conversation

estroz commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adirio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

estroz commented Mar 4, 2020

Uh oh!

Adirio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

estroz commented Mar 23, 2020

Uh oh!

camilamacedo86 commented Mar 23, 2020

Uh oh!

camilamacedo86 commented Mar 24, 2020

Uh oh!

k8s-ci-robot commented Mar 24, 2020

Uh oh!

camilamacedo86 commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilamacedo86 commented Mar 25, 2020

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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

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

estroz commented Mar 3, 2020 •

edited

Loading

camilamacedo86 commented Mar 24, 2020 •

edited

Loading