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

Allow every command to overwrite project-configured plugins #1941

Closed
Adirio opened this issue Jan 13, 2021 · 8 comments · Fixed by #2060
Closed

Allow every command to overwrite project-configured plugins #1941

Adirio opened this issue Jan 13, 2021 · 8 comments · Fixed by #2060
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Milestone

Comments

@Adirio
Copy link
Contributor

Adirio commented Jan 13, 2021

When resolivng which plugins will handle each command, we currently have 3 sources for the project version and plugin keys: project configuration file, flags or default values.

Init commands will not have a project configuration file, so the precedence is clear, flags override default values. However, the rest of the commands require a project configuration so they have 2 sources that are at the same level, flags and project configuration, both overriding the default values. If both flags and project configuration are present, the current approach is to fail in case they are different. This makes total sense for the project version, as you can't run a create command for project version 3 on a project version , obviously. But in the case of plugins, we may want to allow them to be different.

Let's say that we create a plugin that modifies the controller.go file to insert inside the reconcile function some scaffold code and/or that creates a controller_test.go scaffold. We may want to use this plugin for some of the controllers generated in this project but not for all of them.

I would allow command calls' --plugin flag to temporaly override the layout field set in the project configuration file. And by temporaly override I mean that this command call will be called with the values of the flag but the project configuration layout field will not be modified, and therefore, succesive command calls will still use the plugins configured during project initialization.

@Adirio Adirio added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 13, 2021
@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 9, 2021

in the EP you added the following example:

kubebuilder init --plugins=go.kubebuilder.io/v3
kuebebuilder create api --plugins=go.kubebuilder.io/v3,extra.kubebuilder.io/v1 ...

So, I understand that it allow executing the create api of go.kubebuilder.io/v3 and then the same subcommand of extra.kubebuilder.io/v1. Am I right? then, I have a few questions:

1 - Why we need to re-inform go.kubebuilder.io/v3 since it is the project layout?
2 - Also, how you will check that all inputs required for A and B were provided before to do the execution?
3- What is the difference between your suggestion with phase 2 plugin? I mean, phase 2 would allow us to call more than one plugin in the same execution. So, what is the motivation of this EP?
4- Could you please add some user stories to illustrate the problems that you are trying to solve with? ( template As a <type of user>, I can <some goal> so that <some reason>

@Adirio
Copy link
Contributor Author

Adirio commented Feb 9, 2021

So, I understand that it allow executing the create api of go.kubebuilder.io/v3 and then the same subcommand of extra.kubebuilder.io/v1. Am I right?

Yes, the example considers plugin chaining which is not available in plugin phase 1. So there are two different topics, first plugin chaining and second replacing the plugins for a specific command call. This Issue talks about the second point. The first point is proposed in plugin phase 1.5.

1 - Why we need to re-inform go.kubebuilder.io/v3 since it is the project layout?

What you are doing is replacing the plugins just for that specific call. It would also be useful for removing a plugin if you dont want it for that command.

2 - Also, how you will check that all inputs required for A and B were provided before to do the execution?

I understand that by A and B you mean both plugins.
It won't do anything special in that sense. It will just execute that command call as if the layout was different, but just for that command call. This means that each plugin will validate itself the same way they are doing right now.

3- What is the difference between your suggestion with phase 2 plugin? I mean, phase 2 would allow us to call more than one plugin in the same execution. So, what is the motivation of this EP?

The idea of this issue is to allow to use a certain set of plugins just for one call. Imagine we have a plugin that creates tests for the controllers, for example. But we don't want to set it for all of out controllers, so we don't set that plugin in kb init. We just use that plugin for certain kb create api calls.

4- Could you please add some user stories to illustrate the problems that you are trying to solve with? ( template As a <type of user>, I can <some goal> so that <some reason>

As a operator/controller developer I have 5 different APIs with their corresponding controllers. One of them uses the declarative pattern, the other 2 don't. I want to be able to set the declarative pattern plugin ONLY for the corresponding kb create api call, while the other four don't use that plugin.

@camilamacedo86 camilamacedo86 added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Feb 9, 2021
@camilamacedo86 camilamacedo86 added this to the next milestone Feb 9, 2021
@camilamacedo86 camilamacedo86 added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Feb 9, 2021
@camilamacedo86
Copy link
Member

As we discussed in the kb meeting shows that it would require changes in the PROJECT file (config) to track what info was scaffold with each plugin. And then, because of this, it might only be addressed for v4.0.0.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 13, 2021

I'd like to only summarize here the full discussion and the common ground that we reach about this issue to address all concerns raised.

This issue would be implemented as a follow to #1991. Where it will allow us to call a superset of plugins for a specific command. It will allow a user to decide what execution should or not receive the additional plugins scaffolds. By using the declarative.kubebuilder.io/v1 from #1991, in this scenario, users would able to decide what API should or not use the addon pattern instead of applying it for the full project.

Example

Config:

layout: layout: go.kubebuilder.io/v3

Then when we call:
$ kubebuilder create api --group crew --version v1 --kind Captain --plugins="declarative.kubebuilder.io/v1"

It will execute the create api command implemented for the default layout go.kubebuilder.io/v3 and then, the create API subcommand implemented in declarative.kubebuilder.io/v1 will be called for the specific data-informed.

In order to allow us to troubleshoot and still able to re-create the project from the config data then; the plugin info will be stored in the PROJECT file in the preexistent attribute plugins in the same way that the emulation of plugins phase 2 ( custom scaffolds) has been done in SDK (see here):

plugins:
    declarative.kubebuilder.io/v1: {}

Also, if the plugin informed has additional args/flags then this data will be tracked in the specific plugin map such as:

plugins:
    declarative.kubebuilder.io/v1: 
        domain: example.io
        group: crew
        kind: Captain
        version: v1
        flag1: value  // specific flags for the plugin informed in the --plugins
        flag2: value  // specific flags for the plugin informed in the --plugins

Note that the definition for it would store the GVK + the additional plugin flags since the GVK is a unique identifier for the resources scaffold for the project.

Also, to ensure this scenario with the addon/declarative plugin we can call it for only one of the many API's scaffolds in the generate_testdata of the project-v3.

@Adirio
Copy link
Contributor Author

Adirio commented Feb 13, 2021

Just adding onto Camila's comment:
Camila prefers the approach were only additional plugins can be set, and therefore --plugins in subsequent command calls will add them to those used in init.
I prefer the approach of writting all the plugins in --plugins in subsequent calls (we can also offer a --extra-plugins that works as Camila described) so that a concrete command call can choose also to remove some of the plugins, not only add some.

@camilamacedo86
Copy link
Member

To clarify:

Example (Adirio suggestion with both flags)

  • Config: layout:go/v2
  • Command: $ kubebuilder create api --group crew --version v1 --kind Captain --plugins="go/v3,declarative.kubebuilder.io/v1"

It would execute the create api implement go/v3 and then the wrapper declarative.kubebuilder.io/v1.

The default behaviour described in #1941 (comment) would be done via --extra-plugins instead.

IMO

It shows problematic since we would allow the scaffold to create api using the go/v3 for a project scaffolded using go/v2 which would not work well. The worst part is that we can perform the scaffold succefully and then, users will face issues only in other/post actions such as when trying to run make manifests.

I cannot see a current requirement that justifies we bring these complexities and add both flags. Also, it shows brings confusion and makes its usage less intuitive for the users and us.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Mar 7, 2021

IMO: As described above, we make ask for the users inform again the default global plugins defined in the layout might not to be the best user experience and can bring confusion. Also, I still think that it might bring some technical complexities such as to track the plugin info or allow more compatible possibilities which we could avoid.

In this way, I would prefer/vote in the overwrite means run layout plugins + the plugins informed in the chain. Then, we would track in attr plugins only what was informed in the chain.

However, since you are working on it and if you think that still easier for us to achieve the goal requires at least in this first moment inform all plugins instead. Then, I have no big objections to it.

However, to close this one we will need to address the points discussed in #2060 (comment) a follow-up. After 1.5 it will still be missing:

  • By informing --plugins to the subcommand it is missing track the info in the PROJECT file.
  • Add a help text for declarative plugin to allow us to check how the help text will be with $ kubebuilder create api --plugins="go/v3,declarative"
  • shows the --plugins option for all subcommands with the cli help

@Adirio
Copy link
Contributor Author

Adirio commented Mar 8, 2021

The main advantage from overiding plugins instead of adding plugins is the following:

Overriding plugins:

kubectl init --plugins go/v3
kubectl create api --plugins go/v3,declarative ...

is equivalent to incrementing plugins:

kubectl init --plugins go/v3
kubectl create api --plugins declarative ...

but overriding plugins:

kubectl init --plugins go/v3,declarative
kubectl create api --plugins go/v3 ...

cannot be achieved with incrementing plugins.

P.S.: the three points listed and their fulfillment is being tracked in the linked comment.

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/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
2 participants