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.

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

Jump to bottom

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

Merged
merged 1 commit into from
Apr 17, 2020
Merged

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

merged 1 commit into from
Apr 17, 2020

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Apr 8, 2020 •
edited

Per discussion in #1403 (comment):

[H]aving 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.

There still may be a use case for phase 1 global --plugins: I scaffold a project with --plugins=go-x then want to create an API using plugin go-y, which "understands" a project initialized by go-y; without a global --plugins flag I would have to change my config's layout value.

Thoughts?

/cc @DirectXMan12 @mengqiy @joelanford

@k8s-ci-robot 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 Apr 8, 2020
@estroz
Copy link
Contributor Author

estroz commented Apr 8, 2020

/hold

while we discuss

@k8s-ci-robot 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 8, 2020
@estroz estroz closed this Apr 9, 2020
@estroz estroz reopened this Apr 9, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2020
@estroz estroz mentioned this pull request Apr 9, 2020
Merged
1 task
@estroz estroz force-pushed the refactor/remove-plugins-global-flag branch from 739dfd7 to 32a970e Compare April 9, 2020 17:26
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2020
camilamacedo86
camilamacedo86 reviewed Apr 14, 2020
View reviewed changes
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.

I am ok with these changes. 👍
1 nit only: I think is better use len(string) > 0 than !=""

@estroz estroz force-pushed the refactor/remove-plugins-global-flag branch from 32a970e to 8c4375c Compare April 14, 2020 17:29
camilamacedo86
camilamacedo86 reviewed Apr 14, 2020
View reviewed changes
pkg/cli/cli.go
}
c.cliPluginKeys[pluginName] = pluginVersion
}
c.cliPluginKey = strings.TrimSpace(c.cliPluginKey)
Copy link
Member

Choose a reason for hiding this comment

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

👍

camilamacedo86
camilamacedo86 approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited

Choose a reason for hiding this comment

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

It is :

/lgtm

for me.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2020
@camilamacedo86
Copy link
Member

Hi @estroz missing run make generate.

@estroz
pkg/cli: move --plugins to init, since phase 1 only allows one plugin
8707feb 
to be used for scaffolding at a time, and a config's 'layout' value
should be used for other commands. Also change the implementation of
resolvePluginsByKey to only match one key, and add tests
@estroz estroz force-pushed the refactor/remove-plugins-global-flag branch from 8c4375c to 8707feb Compare April 14, 2020 21:51
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2020
@camilamacedo86
Copy link
Member

/approve

@camilamacedo86
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 14, 2020
@estroz estroz mentioned this pull request Apr 15, 2020
Merged
@mengqiy
Copy link
Member

mengqiy commented Apr 17, 2020

/approve
/lgtm
Feel free to go ahead and merge this when you are ready.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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,mengqiy]

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

@estroz
Copy link
Contributor Author

estroz commented Apr 17, 2020

/hold cancel

@k8s-ci-robot 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 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 24cddcc into kubernetes-sigs:feature/plugins-part-2-electric-boogaloo Apr 17, 2020
@estroz estroz deleted the refactor/remove-plugins-global-flag branch April 17, 2020 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mengqiy mengqiy mengqiy left review comments

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

@joelanford joelanford Awaiting requested review from joelanford

Assignees

@mengqiy mengqiy

@camilamacedo86 camilamacedo86

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@estroz @camilamacedo86 @mengqiy @k8s-ci-robot