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

pkg/cli: only allow one default plugin per project version #1542

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Jun 1, 2020

A CLI's default plugins now contain only one plugin per project version, and the Option returned by WithDefaultPlugins will return an error if multiple are found. The one-to-one mapping identifies which plugin that should be run when no plugin key is passed via layout or --plugins.

  • pkg/cli: change the type of cli.defaultPluginsFromOptions such that
    it maps one project version to one plugin.

Relates to #1536.

/cc @joelanford @camilamacedo86 @DirectXMan12

version, and the Option returned by WithDefaultPlugins will return
an error if multiple are found. The one-to-one mapping disambiguates
which plugin should be run when no plugin key is passed via 'layout'
or '--plugins'.

pkg/cli: change the type of cli.defaultPluginsFromOptions such that
it maps one project version to one plugin.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 1, 2020
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Hi @estroz,

Currently, KB has just 1 plugin version which makes these changes pass successfully in the CI. However, if we apply here the changes made in the commit e38b06b from the PR #1498 where we are adding a new plugin version it will NOT work well as expected. See:

$ make generate
rm -rf /Users/camilamacedo/go/bin/controller-gen
make generate-testdata
make[1]: Entering directory '/Users/camilamacedo/go/src/sigs.k8s.io/kubebuilder'
GO111MODULE=on ./generate_testdata.sh
~/go/src/sigs.k8s.io/kubebuilder ~/go/src/sigs.k8s.io/kubebuilder
Starting to generate projects with version 2
Generating project-v2
go: creating new go.mod: module sigs.k8s.io/kubebuilder/testdata/project-v2
initializing project-v2 ...
2020/06/01 14:08:26 broken pre-set default plugins: project version "3-alpha" already has plugin "go.kubebuilder.io/v2.0.0"
make[1]: *** [Makefile:63: generate-testdata] Error 1
make[1]: Leaving directory '/Users/camilamacedo/go/src/sigs.k8s.io/kubebuilder'
make: *** [Makefile:58: generate] Error 2

So, IMO let's merge the PR #1498 and the fix #1538 and then, we will have the KB with more the 1 plugin version implemented and checked by the CI. After that will be easier to do improvements and changes as you are suggesting without the risk to introduce new issues for this scenario.

@estroz
Copy link
Contributor Author

estroz commented Jun 1, 2020

@camilamacedo86 #1498 should fail with this PR because both v2 and v3 plugins from that PR support an overlapping set of project versions, which this PR aims to avoid. The goal of this PR is to say "a user will use this plugin if they have not specified one for their project's version". Right now, having more than one is ambiguous given how a CLI is constructed.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jun 1, 2020

Hi @estroz,

So, you are saying that the following scenario is invalid?

// When has more than one supportable plugin. E.g:
// - go.kubebuilder.io/v2.0.0 which is supported by V2 and V3
// - go.kubebuilder.io/v3.0.0 which is supported by V3

If yes, should we not change the var supportedProjectVersions = []string{config.Version2, config.Version3Alpha}. I mean, should it really be an array? Or is your idea that the following scenario would be valid and because of this we should keep the array?

// - go.kubebuilder.io/v2.0.0 which is supported by V2 and V3
// - go.kubebuilder.io/v3.0.0 which is supported by V4

@estroz
Copy link
Contributor Author

estroz commented Jun 1, 2020

Project versions 2 and 3 should have different default plugins. Realistically the plugin for version 2 should be “special” since there is no layout key for that project scheme. See #1537 for an example.

@camilamacedo86
Copy link
Member

Hi @estroz,

I thought that we would allow the multi-project version. However, thinking about keeping it simple and reduce the complexity I agree with you. It shows the best approach to move forward.

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Checked that it worked as proposed to work. See:


2020/06/01 14:08:26 broken pre-set default plugins: project version "3-alpha" already has plugin "go.kubebuilder.io/v2.0.0"
make[1]: *** [Makefile:63: generate-testdata] Error 1
make[1]: Leaving directory '/Users/camilamacedo/go/src/sigs.k8s.io/kubebuilder'
make: *** [Makefile:58: generate] Error 2

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2020
@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit 62af06f into kubernetes-sigs:master Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants