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

✨ Make the edit command to be a plugin. #1691

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

prafull01
Copy link
Contributor

@prafull01 prafull01 commented Sep 23, 2020

Description of the change
Moved the edit commands to the plugins just like KB init , KB create api and KB create webhook plugins for v3+.
Kept the v2 behavior exactly same.

Motivation for the change
It would be nice to have the edit plugin which can be extending as well. Also, it is in sync with other commands which have their own extendible plugins. Making edit command a plugin gives us feasibility to extend the functionality of edit command same as any other command in kubebuilder. For example: In #1640 RFE whenever we move the project layout from single group to multi group, the api/ folder (created in single group project layout) should be moved to apis/ folder.

Fixes #1639

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @prafull01. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@prafull01 prafull01 changed the title ❇️ Moved the edit command to the plugin for v3+ ✨ Moved the edit command to the plugin for v3+ Sep 23, 2020
cmd/main.go Outdated Show resolved Hide resolved
@prafull01 prafull01 force-pushed the edit-plugin branch 2 times, most recently from 6f18b1e to 5ddbcd4 Compare September 24, 2020 11:45
@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 24, 2020
@prafull01
Copy link
Contributor Author

/test pull-kubebuilder-e2e-k8s-1-16-2

pkg/cli/api.go Show resolved Hide resolved
pkg/cli/webhook.go Show resolved Hide resolved
@prafull01
Copy link
Contributor Author

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

1 similar comment
@prafull01
Copy link
Contributor Author

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

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 tested and it worked fine:

$ kubebuilder edit --multigroup
camilamacedo@Camilas-MacBook-Pro ~/go/src/multigroup (test-ocp) $ cat PROJECT 
domain: my.domain
layout: go.kubebuilder.io/v2
multigroup: true
projectName: multigroup
repo: multigroup
version: 3-alpha

Just a few nits otherwise it shows great 👍

pkg/cli/edit.go Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Member

/ok-to-test

@prafull01
Copy link
Contributor Author

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

@camilamacedo86
Copy link
Member

Just for reference. I think the code :

// update dockerfile
if s.multigroup {
str, err = ensureExistAndReplace(
str,
"COPY api/ api/",
`COPY apis/ apis/`)
if err != nil {
return err
}
} else {
str, err = ensureExistAndReplace(
str,
"COPY apis/ apis/",
`COPY api/ api/`)
if err != nil {
return err
}
}
would better fit in the postscafold plugin action.

However, I do not think that it is a blocker for the PR and it also might change soon. So, it for me is

/approve

Great work 👍

@camilamacedo86
Copy link
Member

camilamacedo86 commented Oct 21, 2020

Hi @prafull01,

Could we just update the title for something such as: make the edit command to be a plugin?

@prafull01 prafull01 changed the title ✨ Moved the edit command to the plugin for v3+ ✨ Make the edit command to be the plugin for v3+ Oct 25, 2020
@prafull01
Copy link
Contributor Author

Hi @prafull01,

Could we just update the title for something such as: make the edit command to be a plugin?
Done

@camilamacedo86 camilamacedo86 changed the title ✨ Make the edit command to be the plugin for v3+ ✨ Make the edit command to be an plugin. Oct 25, 2020
@camilamacedo86
Copy link
Member

HI @prafull01,

I update the title for :sparkles: Make the edit command to be a plugin. because it does not change any behaviour. Only move the edit subcommand implementation to be a plugin as we did for creating api and webhooks. So, it is not specific for v3+ our v2+

Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

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

I will let others decide if this is required but you should include some motivation why kubebuilder edit should be considered a plugin instead of an extra command. Examples of what you could do with this for example. What I understand from the code is that it does exaclty the same without setting the layout field of the project configuration file.

Aside from that, a couple NITs to keep the comments same as the other interfaces.

pkg/plugin/interfaces.go Outdated Show resolved Hide resolved
pkg/plugin/interfaces.go Show resolved Hide resolved
pkg/plugin/interfaces.go Show resolved Hide resolved
pkg/cli/cli.go Show resolved Hide resolved
@prafull01 prafull01 changed the title ✨ Make the edit command to be an plugin. ✨ Make the edit command to be a plugin. Oct 27, 2020
pkg/plugin/interfaces.go Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Member

camilamacedo86 commented Oct 27, 2020

Hi @Adirio,

Thank you for your input.

I will let others decide if this is required but you should include some motivation why kubebuilder edit should be considered a plugin instead of an extra command. Examples of what you could do with this for example. What I understand from the code is that it does exaclty the same without setting the layout field of the project configuration file.

The addon and edit should be plugins because all subcommands of kubebuilder should follow this design introduced in the plugin phase 1 in order to keep a standard and consistency in the project whcih is helpful to keep it maintained. Also, it allows other projects to use this plugin whcih is the purpose of the EP https://github.com/kubernetes-sigs/kubebuilder/blob/master/designs/extensible-cli-and-scaffolding-plugins-phase-1.md .

PS.: Both issues (#1543, #1639) were tracked by myself after a chat with @DirectXMan12 over it.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, camilamacedo86, prafull01

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

@camilamacedo86
Copy link
Member

camilamacedo86 commented Oct 27, 2020

It is approved for 2 contributors/reviewers so I think we can move forward with it is as well.
Also, note that it does not change anything for v2+ plugin

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2020
@k8s-ci-robot k8s-ci-robot merged commit 30d9998 into kubernetes-sigs:master Oct 27, 2020
@prafull01 prafull01 deleted the edit-plugin branch October 28, 2020 04:17
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the edit command be an plugin as well
5 participants