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

Move config-gen to a alpha plugin design #2501

Closed
camilamacedo86 opened this issue Feb 1, 2022 · 15 comments
Closed

Move config-gen to a alpha plugin design #2501

camilamacedo86 opened this issue Feb 1, 2022 · 15 comments
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@camilamacedo86
Copy link
Member

What do you want to happen?

Check the possibility to move : https://github.com/kubernetes-sigs/kubebuilder/tree/master/pkg/cli/alpha/config-gen to an alpha plugin under https://github.com/kubernetes-sigs/kubebuilder/tree/master/pkg/plugins

Extra Labels

/kind cleanup

@camilamacedo86 camilamacedo86 added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 1, 2022
@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Feb 1, 2022
@camilamacedo86 camilamacedo86 added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Feb 1, 2022
@camilamacedo86 camilamacedo86 added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Feb 10, 2022
@Kavinjsir
Copy link
Contributor

Kavinjsir commented Mar 14, 2022

Hey @camilamacedo86 I'd like to take on this. Have created a PR for this: #2540
Would you provide me a reivew or some context for the issue so that I can follow up? Thx!

@camilamacedo86
Copy link
Member Author

See that we have a PR that was not finished but init this work: #2099. You might want to check it to continue this work. It was close to being finished I think.

@Kavinjsir
Copy link
Contributor

See that we have a PR that was not finished but init this work: #2099. You might want to check it to continue this work. It was close to being finished I think.

Hey @camilamacedo86 , thx for informing!
Just to clarify:

  1. Plugin design in #2099 provides 3 ways:
    i) kubebuilder init --plugins go-config-gen to scaffold kubebuilder alpha config-gen in Makefile for deploy
    ii) kubebuilder init --plugins go-config-gen --with-kustomize to scaffold kustomize cmds as an alternative
    iii) kubebuilder init --plugins config-gen.go to scaffold a complete project with kubebuilder alpha config-gen in Makefile
    Despite the plugin names, I think option iii fits the default needs as scaffolding without kustomize file. Option ii provides the way for using kustomize. And option i is a standalone one that only scaffolds out the configuration file.
    I think they are good to apply. Can I just go on with all of them?

  2. By saying moving cli/alpha/config-gen to pkg/plugins, do we want to move all the code of cli/alpha/config-gen to pkg/plugins? Maybe similar as the structure of pkg/plugins/golang?

  3. Do we ultimately want to remove command kubebuilder alpha config-gen and use such plugin instead?

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Mar 22, 2022

Hi, @Kavinjsir,

  1. I understand that the option kubebuilder init --plugins go-config-gen --with-kustomize is an additional feature on top of it. (The idea was scaffold go/v3 for example using config-gen instead of the kustomize plugin which is responsible for we do the config/ dir ) So, I am ok with migrating the config-gen to the plugin layout only with (i) and (iii) options and do (ii) in a follow up for example. The config/gen was written by @pwittrock , so we also can ask a hand for him to understand more about it (when/if required). For we get your PR merged I think we ought to ask for his review. But that is the latest step.

For 2. 3. Yes. +1

Also, see that its docs could be moved to a new section: #2542
And that, its sample ought to be generated under the testdata dir: https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Mar 24, 2022

Hi @Kavinjsir,

I would like just to share some context. In the past, we discussed that we would like:

@pwittrock, let me know if I misunderstand something here

@Kavinjsir
Copy link
Contributor

Hey, @camilamacedo86 ,
Thx letting me know!
I will roll back the code with the --with-kustomize flag.
Also, it seems the mode Without --with-kustomize may rely on the layout from the default plugins.
(It has to work based on the api folder. I'm looking on how to handle in standalone mode.)

@camilamacedo86
Copy link
Member Author

@Kavinjsir,

Migrating the config-gen to the plugin layout is not an easy task.
Changing it to also scaffold the kustomize files maybe is hard too.
so, feel free to see. if you want just to migrate for plugin layout and that for a follow-up or not.

@Kavinjsir
Copy link
Contributor

Kavinjsir commented Mar 25, 2022

Hey, @camilamacedo86 ,
Yeah, I do feel it bigger than what a single pr can hold.
Just have a new update based on your comments and with the implementation of the plugin itself.
I will move the file under cli/alpha to plugins/config-gen next.
After that, I think we would have three parts to deal with(according to my understanding on the context you showed me):

  1. Support with-kustomize
  2. Fully migrate the subcommand alpha config-gen to the plugin
  3. Documentation

For 3) documentation, I made some reply on your comment
For 1) and 2), I feel it better to handle in a follow up pr.
Well, if you have any suggestions or recommends that I may try on in the current pr, I will be always glad to go on.

@camilamacedo86
Copy link
Member Author

All that is under plugins respect the. plugin layout/interfaces. Then, (IHMO) we can not move the code implementation from the cmd/alpha under the plugins directory before it is a plugin itself (which is the goal of this task).

However, I do not see any problem in pushing changes to the config-gen where it is now until we are able to achieve the goal of this task. If you would like to change it to make this process easier and after doing this one that is very welcome too.

@Kavinjsir
Copy link
Contributor

Kavinjsir commented Mar 28, 2022

Hi @camilamacedo86 , thx for suggestion!
I'd like to continue on 2) , though I am not quite understanding how...
Would you help me with bit details...like how the plugin is planned to work as alpha config-gen?

My understanding is:

  • kb alpha config-gen can be called any time when the user want to generate a configured yaml to deploy controllers.
  • However, for plugins, they can only function when calling init/create subcommands.

So there are two optional approaches occurred to me:

  1. Provide the layout of what kb alpha config-gen provides either during the call of kb init or kb create api.
  2. During kb init call, compile the code behind kb alpha config-gen to be an executable and put it under bin/ folder (stay in parallel with controller-gen and kustomize, namely config-gen). Then, modify Makefile, such as make deploy: call config-gen to generate the configured yaml file and apply to K8s cluster.

I guess approach 1) requires users to configure config file (e.g: kubebuilderconfiggen.yaml) prior to call kb init or kb create api, so as to apply their configuration to generate the yaml file for the controller.
Approach 2) provides a similar behavior as what current PR does. The difference is it gets rid of kb alpha config-gen by creating an executable in the layout.

I'm not sure if either of them can be good candidate? Would you have some suggestions or maybe help with a better idea?
Thx!

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 26, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 26, 2022
@camilamacedo86
Copy link
Member Author

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jul 27, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2022
@camilamacedo86
Copy link
Member Author

Close this one in favor of : #3000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

4 participants