Plugins Phase 1 #1469

estroz · 2020-04-09T17:20:45Z

This PR contains the implementation of concepts outlined in the CLI plugins phase 1 design doc.

Explanation of package changes/additions:

cmd/: remove all but edit, update, version, and completion subcommands in favor of plugins that implement removed commands. The remaining commands are specific to kubebuilder and should not be reserved command names.
internal/cmdutil: commandOptions -> RunOptions.
pkg/cli: contains the CLI interface and cobra CLI constructors. Most commands
from cmd/ were move here and modified to initialize plugins.
pkg/plugin: Base and *PluginGetter interfaces for v1 and v2 scaffolds. These are kubebuilider's base Go plugins consumed by a CLI.

This branch is a master rebase of the feature branch to which all commits here were reviewed and merged. I've saved that branch's history to preserve merge commits for further review.

TODO's:

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

/kind feature

/cc @DirectXMan12 @mengqiy @joelanford @hasbro17 @camilamacedo86 @Adirio

camilamacedo86 · 2020-04-11T09:23:57Z

cmd/edit.go

-		if c.MultiGroup {
-			return fmt.Errorf("multiple group support can't be enabled for version %s", c.Version)
+func (o *editOptions) Validate() error {
+	if !o.config.IsV2() && !o.config.IsV3() {


Suggested change

if !o.config.IsV2() && !o.config.IsV3() {

if o.config.IsV1() {

Will make easier remove the if to clean the code if/when we remove V1 impl

Not sure if this was the intent, but using !o.config.IsV2() && !o.config.IsV3() means that we would need to explicitly assert that a future version (e.g. v4) supports multi-group. This is probably safer than assuming that all future versions will support multi-group.

Once #1355 is merged I will rebase to remove v1 checks.

cmd/update.go

These changes are the implementation of concepts outlined in designs/extensible-cli-and-scaffolding-plugins-phase-1.md * cmd/: remove all but 'edit' and 'update' subcommands in favor of plugins that implement removed commands * internal/cmdutil: RunOptions configure Run() to execute a command in a generic manner * pkg/cli: CLI interface and cobra CLI constructors. Most commands from cmd/ were move here and modified to initialize plugins * pkg/plugin: Plugin and PluginGetter interfaces for v1 and v2 scaffolds. These are kubebuilider's base Go plugins consumed by pkg/cli.CLI

pkg/plugin: validation for plugin version strings pkg/internal/validation: project-wide validation logic; move pkg/model/resource/validation.go functions here

internal type pkg/plugin: InjectConfig passes a Config object to a plugin that the plugin can modify instead of having the plugin load/save the plugin itself pkg/cli: manage initialization, loading, and saving of a Config object, and inject the Config object into plugins internal/cmdutil: remove LoadConfig and Config object parameters from RunOptions interface cmd: rewrite edit/update to respect new command runner interface

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

'plugins' key, with each plugin's config keyed by its plugin key, and an encoder and decoder for plugin configuration objects. Add marshal/unmarshal functions for a Config internal/config: use Config marshalling/unmarshalling to read/write a config, and add tests

… a version bump. This new version will be '3-alpha', which will eventually be bumped to '3' once the below config changes have stabilized. *: add tests and checks for v3 where appropriate

testdata: add v3 test data

the correct command name pkg/cli: use layout or cliPluginKeys in error messages if no plugin for a command is found

estroz · 2020-04-15T19:02:00Z

/hold

so this isn't merged before 2+ reviews happen.

camilamacedo86

It shows be missing :

rebase with the branch
rebase with master
run make generate

estroz · 2020-04-18T18:58:03Z

/test pull-kubebuilder-e2e-k8s-1-14-1

go.mod

internal/config/config.go

pkg/cli/api.go

joelanford · 2020-04-23T14:14:20Z

pkg/cli/cli.go

+	// Set base flags that require pre-parsing to initialize c.
+	fs.BoolVarP(&help, helpFlag, "h", false, "print help")
+	fs.StringVar(&c.projectVersion, projectVersionFlag, c.defaultProjectVersion, "project version")
+	fs.StringVar(&c.cliPluginKey, pluginsFlag, "", "plugins to run")


Is this plural for forward-compatibility reasons? Should this use fs.StringSliceVar()?

For now, should we check to ensure only 1 plugin is provided?

For forward compatibility reasons yes. I have it as a string so users aren't confused about what type (string vs string list) they can pass to --plugins. Once phase 2 is done we can update the flag type without actually breaking the flag API.

pkg/cli/cli.go

joelanford · 2020-04-23T14:56:03Z

cmd/edit.go

-		if c.MultiGroup {
-			return fmt.Errorf("multiple group support can't be enabled for version %s", c.Version)
+func (o *editOptions) Validate() error {
+	if !o.config.IsV2() && !o.config.IsV3() {


Not sure if this was the intent, but using !o.config.IsV2() && !o.config.IsV3() means that we would need to explicitly assert that a future version (e.g. v4) supports multi-group. This is probably safer than assuming that all future versions will support multi-group.

joelanford

I haven't made it all the way through, but I wanted to give you what I have so far. Looks great overall!

internal/config/config_test.go

pkg/cli/api.go

pkg/cli/cli_test.go

pkg/cli/init.go

pkg/model/config/config.go

joelanford · 2020-04-23T15:31:21Z

pkg/model/config/config.go

 )

 // Scaffolding versions
 const (
 	Version1 = "1"
 	Version2 = "2"
+	Version3 = "3-alpha"


What happens when we have 3-beta and 3? Should this const be called Version3Alpha?

And go through a deprecation process from 3-alpha and 3-beta following a few releases of 3, or remove them once 3 is out?

As long as we don't break anything from 3-alpha to 3-beta to 3, perhaps we can auto-promote the projectVersion?

That's what I was thinking. That jives with IsV3() instead of direct config.Version == VersionN comparison too.

pkg/model/config/config.go

pkg/model/config/config_test.go

test/e2e/v3/e2e_suite.go

pkg/cli/webhook.go

test/e2e/v3/e2e_test.go

pkg/internal/validation/dns.go

DirectXMan12

mostly nits. Many should be considered nonblocking.

pkg/cli/init.go

pkg/model/config/config.go

DirectXMan12 · 2020-04-27T20:40:47Z

pkg/plugin/plugin.go

+}
+
+// Key returns a unique identifying string for a plugin's name and version.
+func Key(name, version string) string {


nit: just do type Key string and make some of the split, etc be methods on that?

estroz · 2020-04-28T02:24:53Z

Post-review updates made in #1485, commits will be cherry-picked here after merging.

all encode/decode operations on a Config methods

estroz · 2020-04-28T15:22:44Z

/test pull-kubebuilder-e2e-k8s-1-17-0

camilamacedo86

It shows OK for me. I will set approve. Please wait until we have the green flag to merge and at least more one ok.

/approve

k8s-ci-robot · 2020-04-28T17:03:25Z

[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

… "3-alpha", and update all references pkg/cli: generalize layout key check for future v3 project version types

DirectXMan12 · 2020-04-28T20:53:07Z

/lgtm

DirectXMan12 · 2020-04-28T20:53:31Z

/hold cancel

estroz · 2020-04-28T20:53:55Z

/hold cancel

k8s-ci-robot requested review from Adirio, camilamacedo86, DirectXMan12, hasbro17, joelanford and mengqiy April 9, 2020 17:20

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 9, 2020

camilamacedo86 reviewed Apr 11, 2020

View reviewed changes

cmd/update.go Outdated Show resolved Hide resolved

hasbro17 mentioned this pull request Apr 13, 2020

Add plugins based CLI operator-framework/operator-sdk#2827

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2020

estroz added 9 commits April 14, 2020 14:24

pkg/cli: validate cli struct's fields

883e8c6

pkg/plugin: validation for plugin version strings pkg/internal/validation: project-wide validation logic; move pkg/model/resource/validation.go functions here

cli: check if project is v1 when validating default plugins

e4b8d15

Any changes that break 'PROJECT' file backwards-compatibility require…

5d5e7d2

… a version bump. This new version will be '3-alpha', which will eventually be bumped to '3' once the below config changes have stabilized. *: add tests and checks for v3 where appropriate

test/e2e: add v3 tests

706019b

testdata: add v3 test data

pkg/plugin: inject command name from Context so help messages contain

cd2d948

the correct command name pkg/cli: use layout or cliPluginKeys in error messages if no plugin for a command is found

estroz force-pushed the chore/rebase-plugins branch from a067400 to cd2d948 Compare April 14, 2020 21:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2020

estroz mentioned this pull request Apr 15, 2020

Clean Code - Remove V1 implementation #1355

Merged

estroz marked this pull request as ready for review April 15, 2020 19:01

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 Apr 15, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2020

camilamacedo86 reviewed Apr 18, 2020

View reviewed changes

camilamacedo86 reviewed Apr 20, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

camilamacedo86 reviewed Apr 20, 2020

View reviewed changes

internal/config/config.go Show resolved Hide resolved

joelanford reviewed Apr 23, 2020

View reviewed changes

camilamacedo86 reviewed Apr 24, 2020

View reviewed changes

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

camilamacedo86 reviewed Apr 24, 2020

View reviewed changes

test/e2e/v3/e2e_test.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Apr 24, 2020

View reviewed changes

pkg/internal/validation/dns.go Outdated Show resolved Hide resolved

DirectXMan12 requested changes Apr 27, 2020

View reviewed changes

estroz mentioned this pull request Apr 28, 2020

plugins: cleanup from #1469 #1485

Closed

estroz requested review from joelanford, camilamacedo86 and DirectXMan12 April 28, 2020 02:25

estroz added 2 commits April 28, 2020 08:10

*: fix validation of default values, update license years, and make

1ee71ec

all encode/decode operations on a Config methods

pkg: make added unit tests into ginkgo/gomega tests

a639be0

camilamacedo86 approved these changes Apr 28, 2020

View reviewed changes

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

pkg/model/config: use Version3Alpha variable name for project version…

8093011

… "3-alpha", and update all references pkg/cli: generalize layout key check for future v3 project version types

k8s-ci-robot assigned DirectXMan12 Apr 28, 2020

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2020

k8s-ci-robot merged commit d6e2b99 into kubernetes-sigs:master Apr 28, 2020

estroz deleted the chore/rebase-plugins branch April 28, 2020 22:07

camilamacedo86 mentioned this pull request Feb 18, 2021

New Release - proposing 3.0.0 stable #2029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugins Phase 1 #1469

Plugins Phase 1 #1469

estroz commented Apr 9, 2020 •

edited

Loading

camilamacedo86 Apr 11, 2020 •

edited

Loading

camilamacedo86 Apr 11, 2020 •

edited

Loading

joelanford Apr 23, 2020

estroz Apr 27, 2020

estroz commented Apr 15, 2020

camilamacedo86 left a comment

estroz commented Apr 18, 2020

joelanford Apr 23, 2020

estroz Apr 27, 2020

joelanford Apr 23, 2020

joelanford left a comment

joelanford Apr 23, 2020

DirectXMan12 Apr 27, 2020

estroz Apr 27, 2020 •

edited

Loading

joelanford Apr 28, 2020

estroz Apr 28, 2020

estroz Apr 28, 2020

DirectXMan12 left a comment

DirectXMan12 Apr 27, 2020

estroz Apr 27, 2020

DirectXMan12 Apr 28, 2020

estroz commented Apr 28, 2020

estroz commented Apr 28, 2020

camilamacedo86 left a comment •

edited

Loading

k8s-ci-robot commented Apr 28, 2020

DirectXMan12 commented Apr 28, 2020

DirectXMan12 commented Apr 28, 2020

estroz commented Apr 28, 2020

	if !o.config.IsV2() && !o.config.IsV3() {
	if o.config.IsV1() {

Plugins Phase 1 #1469

Plugins Phase 1 #1469

Conversation

estroz commented Apr 9, 2020 • edited Loading

camilamacedo86 Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

camilamacedo86 Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz commented Apr 15, 2020

camilamacedo86 left a comment

Choose a reason for hiding this comment

estroz commented Apr 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz commented Apr 28, 2020

estroz commented Apr 28, 2020

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 28, 2020

DirectXMan12 commented Apr 28, 2020

DirectXMan12 commented Apr 28, 2020

estroz commented Apr 28, 2020

estroz commented Apr 9, 2020 •

edited

Loading

camilamacedo86 Apr 11, 2020 •

edited

Loading

camilamacedo86 Apr 11, 2020 •

edited

Loading

estroz Apr 27, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading