-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
📖 proposal for new plugin to generate code ( deploy-image.go.kubebuilder.io/v1beta1
)
#1803
📖 proposal for new plugin to generate code ( deploy-image.go.kubebuilder.io/v1beta1
)
#1803
Conversation
fded799
to
1498b06
Compare
1498b06
to
026b540
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
026b540
to
b65cb44
Compare
This comment has been minimized.
This comment has been minimized.
/lgtm |
This comment has been minimized.
This comment has been minimized.
a137ee3
to
3318884
Compare
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer: I didn't read thoroughly the implementation examples.
I agree with the goals of this EP. My two cents on the open questions at the end:
-
Should we allow to scaffold the code for an API that is already created for the project?
I agree that we should require both the API and the controller to be created with the same command call as both parts are required for this plugin. -
Should we support StatefulSet and Deployments?
Starting with one and increasing the supported types incrementally sounds good to me. -
Could this feature be useful to other languages or is it just valid to Go based operators?
This plugin would mainly modify*_types.go
and*_controller.go
which are obviously only relevant for go based operators. In a future, if other languaged based operators are supported (either officially or by the community) this plugin could be used as reference to create an equivalent one for their language. Therefore, it should probably be a "subdomain" ofgo.kubebuilder.io
NIT: I think we need to come up with a better name for this plugin. Generate code seems pretty imprecise in a scaffolding project. The plugin is about good practices and image deploying. Do you like good-practices.go.kubebuilder.io
or deploy-image.go.kubebuilder.io
? I don't really like the first for a plugin name despite being descriptive.
3318884
to
c94c2d6
Compare
Hi @Adirio, Thank you for your comments and review. I adopted the name |
c94c2d6
to
6ee4337
Compare
deploy-image.go.kubebuilder.io/v1beta1
) to generate code
deploy-image.go.kubebuilder.io/v1beta1
) to generate code deploy-image.go.kubebuilder.io/v1beta1
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will let other mantainers the chance to take a look before merging, but LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm, and I agree with the responses @Adirio has in #1803 (review).
A few things before I approve this proposal:
- This proposal is dependent on at least phase 1.5, if not phase 2, plugins, so it can't be implemented or tested until the that requirement is complete.
- Will this plugin live in the kubebuilder repo? I am ok with that, but the test matrix will start blowing up if more plugins are added. Something to be aware of. IMO I'd like to see a
kubebuilder-plugins
repo, with submodules for each plugin.
2. Should we support StatefulSet and Deployments? | ||
The idea is we start it by using a Deployment. However, we can improve the feature in follow-ups to support more default types of scaffolds which could be like `kubebuilder create api --group=crew --version=v1 --image=myexample:0.0.1 --kind=App --plugins=deploy-image.go.kubebuilder.io/v1beta1 --type=[deployment|statefulset|webhook]` | ||
|
||
3. Could this feature be useful to other languages or is it just valid to Go based operators? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since kubebuilder is only concerned with Go projects and the proposal discusses Go-only changes, I think the answer to this question is simply "no".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to leave the answer to clarify some questions raised. This plugin is only valid for go. However, the same idea could be used as an example.
|
||
```yaml | ||
plugins: | ||
deploy-image.go.kubebuilder.io/v1beta1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This configuration implies that phase 2 is required to implement this plugin, since the 1.5 proposal doesn't discuss adding plugin configs to plugins
. @Adirio that's correct right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project-version v3 already supports that field. Nothing specific was defined for plugin phase 1.5 but it doesn't prevent it either. So I would say it doesn't require phase 2 (supporting external binaries is something we still haven't properly designed AFAIK) but it definetely requires phase 1.5.
Camila and me have already recognized the test matrix issue (there are already some permutations that we are not testing like plugin go/v2 with project-version v3) and adding plugins will make it even a greater issue. I already considered a separate repo (I called it |
This plugin address a need that has been requested in SDK for mone than one year. For example operator-framework/operator-sdk#2158. Also, it addresses a discussion made with @pwittrock as well in order to make the things easier for the end-users address the most common causes. So, this one would be born with "official/beta" support. In the long term yes. However, see that currently, Kubebuilder has not too many plugins yet. And then, and the preliminary support for plugins did not indeed released. In this way, at this moment, in POV it shows a little Premature Optimization care about it. Note that the issue #2016(plugin phase 2) will check the possibility of the plugins be as separate binaries that can be discovered by the Kubebuilder CLI binary via user-specified plugin file paths. Then, the discussion over the best approach to dealing with many plugins and if they should or not leave in the kubebuilder repository would be better addressed after that Is it make sense? can we agree on that? However, I will clarify it in the EP. Also, the other point raised by @estroz as well regarding its blockers. |
6ee4337
to
ab1ae6b
Compare
ab1ae6b
to
53b2cf6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite the open issues, this is ok for now.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Adirio, camilamacedo86, estroz 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 |
Description
Motivation