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

⚠️ Plugin phase 1.5 implementation #2060

Merged
merged 1 commit into from
Mar 18, 2021
Merged

⚠️ Plugin phase 1.5 implementation #2060

merged 1 commit into from
Mar 18, 2021

Conversation

Adirio
Copy link
Contributor

@Adirio Adirio commented Mar 3, 2021 •
edited

Design doc

It would solve the following issues.

NOTE:
To ensure that all changes proposed here also works with the consumer (SDK), operator-framework/operator-sdk#4581 was created.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 3, 2021
@k8s-ci-robot k8s-ci-robot added 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. labels Mar 3, 2021
@camilamacedo86

This comment has been minimized.

Sign in to view
@Adirio
Copy link
Contributor Author

Adirio commented Mar 3, 2021

I just realized this has a +5% coverage 🎉

@jmrodri
Copy link
Contributor

jmrodri commented Mar 3, 2021

/cc @jmrodri

@k8s-ci-robot
Copy link
Contributor

@jmrodri: GitHub didn't allow me to request PR reviews from the following users: jmrodri.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jmrodri

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jmrodri
jmrodri approved these changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

@jmrodri: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Adirio
Copy link
Contributor Author

Adirio commented Mar 5, 2021

/hold

Ongoing discussion about accepting this before or after v3.0.0, until thats it solved I'm holding to prevent a mistake-merge

@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 Mar 5, 2021
camilamacedo86
camilamacedo86 reviewed Mar 5, 2021
View reviewed changes
pkg/cli/options.go Outdated Show resolved Hide resolved
@Adirio Adirio force-pushed the plugin-phase-1.5 branch 3 times, most recently from 3cb9df6 to e5eea91 Compare March 6, 2021 12:07
camilamacedo86
camilamacedo86 reviewed Mar 6, 2021
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.

.

camilamacedo86
camilamacedo86 approved these changes Mar 6, 2021
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.

Hi @Adirio,

I am very happy with all changes here. 🥇 I also tested them locally and I have only 1 observation. I do not think that we can close #1941 with this one. See: #2060 (comment)

Otherwise, it has my OK as well for we move forward.

@camilamacedo86
Copy link
Member

@Adirio

This pr will also closes : #1543

This was referenced Mar 7, 2021
Closed
Closed
@camilamacedo86
Copy link
Member

/test pull-kubebuilder-e2e-k8s-1-15-12

@Adirio Adirio force-pushed the plugin-phase-1.5 branch 2 times, most recently from 7eebe8b to 09f047a Compare March 8, 2021 10:00
@Adirio Adirio added this to the v3.0.0 milestone Mar 16, 2021
@estroz estroz mentioned this pull request Mar 16, 2021
Merged
DirectXMan12
DirectXMan12 reviewed Mar 17, 2021
View reviewed changes
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

couple of nits. Main comment is there's this pattern of iterating over the commands, checking if they match and interface, trying to do something if they do, and skipping them if that fails. We should have a helper for that -- I think it'd make the code a lot clearer

pkg/cli/cmd_helpers.go Outdated Show resolved Hide resolved
pkg/cli/cli.go Outdated Show resolved Hide resolved
pkg/cli/cmd_helpers.go Outdated Show resolved Hide resolved
pkg/cli/edit.go Outdated Show resolved Hide resolved
pkg/cli/cmd_helpers.go Outdated Show resolved Hide resolved
@Adirio Adirio force-pushed the plugin-phase-1.5 branch 2 times, most recently from 25b76f6 to fd52d3b Compare March 17, 2021 12:37
@camilamacedo86 camilamacedo86 mentioned this pull request Mar 17, 2021
Closed
@DirectXMan12
Copy link
Contributor

DirectXMan12 commented Mar 17, 2021 •
edited

/lgtm

can refactor later if we need to

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 17, 2021
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 18, 2021
@Adirio Adirio force-pushed the plugin-phase-1.5 branch 3 times, most recently from cb20ff2 to 25dcf06 Compare March 18, 2021 08:31
@Adirio
Plugin phase 1.5 implementation
f2e1edb 
Signed-off-by: Adrian Orive <adrian.orive.oneca@gmail.com>
@camilamacedo86
Copy link
Member

camilamacedo86 commented Mar 18, 2021 •
edited

All requested changes were done.
It has the approval of @pwittrock, @jmrodri, @DirectXMan12 and me
The PR in SDK as this one has been reviewed by @joelanford, @estroz and me
As @DirectXMan12 described we always can improve things and refractory the stuff latter as well.
@estroz also told to us that would be OK these changes after it receives the changes discussed in the layout field of the config file.

It introduces a good value and is a blocker for we move forward with the other ideas such as #2015, https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/code-generate-image-plugin.md

So, IMO we can move forward with and get it merged
It has my

/lgtm

too.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 18, 2021
@camilamacedo86 camilamacedo86 mentioned this pull request Mar 18, 2021
Closed
@estroz
Copy link
Contributor

estroz commented Mar 18, 2021

/hold cancel
/lgtm

@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 Mar 18, 2021
@k8s-ci-robot k8s-ci-robot merged commit 2983c97 into kubernetes-sigs:master Mar 18, 2021
@Adirio Adirio deleted the plugin-phase-1.5 branch March 22, 2021 20:27
@Adirio Adirio mentioned this pull request Mar 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DirectXMan12 DirectXMan12 DirectXMan12 left review comments

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@jmrodri jmrodri jmrodri approved these changes

@droot droot Awaiting requested review from droot

Assignees

@DirectXMan12 DirectXMan12

@camilamacedo86 camilamacedo86

@estroz estroz

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.

Allow every command to overwrite project-configured plugins make addon be an plugin
7 participants
@Adirio @camilamacedo86 @jmrodri @k8s-ci-robot @pwittrock @DirectXMan12 @estroz