kubebuilder
kubebuilder copied to clipboard
kubernetes-sigs
:sparkles: implement `create api --workspace` flag for go1.18 style go.work support
This PR attempts to create a backwards-compatible go.work support in go/v3 / go/v4-alpha: Fixes https://github.com/kubernetes-sigs/kubebuilder/issues/2627
The basic implementation uses a flag called --workspace that can be used in create API like
kubebuilder create api --version v1alpha1 --kind TestKind --group group --workspace, which leads to a go.work being created with the following scaffold:
go 1.18
use ./api/v1alpha1
//+kubebuilder:scaffold:apis
use .
It also changes the PostScaffolding behavior of the create apicommand by using go mod sync instead of go mod tidy to automatically resolve all modules in the workspace. This allows us to not rely on replace statements which would be harder to scaffold and maintain. Obvious downside to this is that it supports go 1.18 and above only.
I want to use this PR first in a draft state to collect input and feedback before progressing it and writing tests.
All existing Makefile commands as well as the already scaffolded go.modstays compatible, however go mod tidywill not work on the core module on its own. (since go.work is necessary to resolve the other modules relative to the base module). Notice that for external developers, you will be able to download the module as normal and also depend on the API modules individually without actually having to import the controller.
Hi @jakobmoellersap. 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.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: jakobmoellersap
Once this PR has been reviewed and has the lgtm label, please assign pwittrock for approval by writing /assign @pwittrock in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Putting this as ready to review, please provide feedback before I proceed further
/hold
there is one big thing open that is requiring the go.mod file contents in the root
@jakobmoellersap I like the ideas behind this change, and I think the work you have done looks great. I just have a few questions:
- My understanding is that this introduces a breaking change for the
create apisubcommand for thego/v3plugin. Is this correct? If so, I think there is probably some hesitation in going this route since thego/v3scaffolding logic is already widely used by both Kubebuilder and Operator SDK users when building Go based operators. - Does this have to be done in the scaffolding of the
create apisubcommand?- Have we considered potentially using the
editsubcommand to facilitate these changes? I think if we could make the changes in theeditsubcommand we wouldn't have to implement any breaking changes to the existingcreate apilogic and rather just introduce a new non-breaking change to theeditsubcommand.
- Have we considered potentially using the
@everettraven Thanks so much for taking a look!
- My understanding is that this introduces a breaking change for the
create apisubcommand for thego/v3plugin. Is this correct? If so, I think there is probably some hesitation in going this route since thego/v3scaffolding logic is already widely used by both Kubebuilder and Operator SDK users when building Go based operators.
I tried to make the entire change backwards-compatible with v3. You should be able to still run all tests and commands like before and you will not even see a difference. It will only show up once you call kubebuilder init --workspace. This is because I changed my original approach from having the flag in the create api command to now be in init instead. There are two changes necessary in the Go API of the module which I tried to solve by creating a second method (for initializing the Scaffolder) with one more parameter and referencing the old methods as deprecated. Since the additional parameter is bool you can even call it with the default value of false and it will behave as the old API. This is also why all API compatibility tests are still green and all cluster tests run just fine.
- Does this have to be done in the scaffolding of the
create apisubcommand?
No => you are right it, doesn't have to be in the create api. Compared to the very beginning, I changed the main scaffolding of the go.work file to be included in the init command. the create api commands will only alter their behavior when a go.work file is detected.
- Have we considered potentially using the
editsubcommand to facilitate these changes? I think if we could make the changes in theeditsubcommand we wouldn't have to implement any breaking changes to the existingcreate apilogic and rather just introduce a new non-breaking change to theeditsubcommand.
I mentioned this as well in the issue and honestly I would like to use it, but I think there are some drawbacks due to the existing kubebuilder scaffolding infrastructure:
- It is not compatible with the
scaffoldingAPI and we would have to use an extra scaffolding that is not done before in kubebuilder based on a command. (right now we only usego mod tidyand make commands and we only do it inPreScaffoldorPostScaffoldafter the actual scaffolding already ran) - If we use the in-code dependency we will have to include it in-code as a dependency and work with their API, namely we would need a combination of the contents of
RepositoryMixInandTemplateMixIn. I think this is an option as well! I am open for feedback here. We would not have to use akubebuilderscaffolding annotation that way.
Your comment regarding using kubebuilder edit is totally valid though, and I think I should consider putting the flag there instead of on kubebuilder init.
One more addition here that I want to put up for discussion:
I tried to make this run with purely go.mod and replace directives. However there is a huge problem with this:
You cannot reference a module that has not yet been pushed to the remote (e.g. Github).
For example:
module github.com/jakobmoellersap/test.com
go 1.18
require (
sigs.k8s.io/controller-runtime v0.12.2
github.com/jakobmoellersap/test.com/api/v1alpha1 0.0.0 //replace with released version later on
//+kubebuilder:scaffold:module-dependency
)
together with
replace github.com/jakobmoellersap/test.com/api/v1alpha1 => ./api/v1alpha1
will break even though we replaced it, no matter if you use go mod tidy or go work sync. This is because of the following error:
go: github.com/jakobmoellersap/test.com/api/[email protected]: reading github.com/jakobmoellersap/test.com/api/v1alpha1/go.mod at revision api/v1alpha1/v0.0.0: git ls-remote -q origin in modcachepath/cache/vcs/d8b5eb86b10c53eaa4c311cd6dd4ef713ab591d0cf9c05f7709ed82b6de97ab5: exit status 128:
remote: Repository not found.
fatal: repository 'https://github.com/jakobmoellersap/test.com/' not found
@jakobmoellersap thanks for taking the time to respond to my previous comment, this all makes sense now. Apologies for missing a couple of the things you mentioned already describing in the issue :sweat_smile: .
I am leaning towards the idea of introducing the functionality into the kubebuilder edit subcommand for the go/v3 plugin. To me this seems a little bit like wanting the ability to "edit" the default scaffolding so that Go workspaces can be utilized - kind of similar to how the kubebuilder edit subcommand can be used to enable multigroup functionality. I am okay with having to make some changes to the scaffolding setup of the edit command to accommodate this type of change under the hood (I think all the subcommands should be able to implement the same scaffolding logic).
One thing I will note, is that if using Go workspaces is a good idea to start setting up as a best practice for the way that operators are scaffolded, I would be okay with adjusting the go/v4-alpha plugin to start using Go workspaces by default (unless we are concerned about the need to use go/v4-alpha scaffolds with a Go version < 1.18).
What do you think?
Thanks for the insight and review guys, I will adopt the PR to make use of the kubebuilder edit command and the PROJECT configuration file.
After spending more time into research, I think workspaces also can have major disadvantages. The best practice way of go is still to use one module per repository. That is to say that I think the pure best practice here would be to
- create separate repositories and check them into vcs separately
- maintain two modules but have to undergo a lot of hands-on effort as you will need to check one module into VCS before checking in the other (otherwise the problem I described above becomes present)
I used the following resources as references: https://github.com/pachyderm/pachyderm/pull/3870 https://github.com/golang/go/wiki/Modules#should-i-have-multiple-modules-in-a-single-repository https://github.com/golang/go/issues/27056 https://go.googlesource.com/proposal/+/master/design/45713-workspace.md#rationale-the-file https://go.dev/doc/modules/managing-source https://go.dev/doc/modules/developing https://go.dev/doc/modules/version-numbers
I also looked way deeper into the go docs now on recommendations of multi-modules, and to be honest, what we are trying here with the PR is probably not an easy to achieve solution in light of the go ecosystem:
From the golang wiki:
Two example scenarios where it can make sense to have more than one go.mod in a repository:
if you have usage examples where the examples themselves have a complex set of dependencies (e.g., perhaps you have a small package but include an example of using your package with kubernetes). In that case, it can make sense for your repository to have an example or _example directory with its own go.mod, such as shown here.
if you have a repository with a complex set of dependencies, but you have a client API with a smaller set of dependencies. In some cases, it might make sense to have an api or clientapi or similar directory with its own go.mod, or to separate out that clientapi into its own repository.
However, for both of those cases, if you are considering creating a multi-module repository for performance or download size for a large set of indirect dependencies, you are strongly encouraged to first try with a GOPROXY, which will be enabled by default in Go 1.13. Using a GOPROXY mostly equals any performance benefits or dependency download size benefits that might otherwise come from creating a multi-module repository.
What I can safely say by now is that go.work files are not meant to be checked into VCS, but rather replace the local replace directives one has to issue if working with multiple modules locally. This also means we're not talking about a pure best practice solution anymore. Instead the recommendation is to use a completely different repository.
I will also not be able to make the main go.mod file work independently without a go.mod file containing a proper replace directive. But we cannot add the replace directive without first checking in the API Module
TLDR: I have a problem now since I think we cannot solve this issue without interacting with the VCS first in the edit command to first create the API Module before initializing the second module and then issuing a replace statement.
The only way I can see currently to solve this is to also interact with the VCS, see https://github.com/golang/go/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository
My understanding now is that the go.work workspaces are meant only for local development purposes as a substitute for the replace directives that one would put into their go.mod files when working with multiple modules. To me, this means that it doesn't quite solve the problem of wanting to be able to have the api sub-module be its own importable module with a smaller set of dependencies.
It is also my understanding that the best practice in its purest form is to have separate repositories - which I don't think will be reasonably feasible.
@jakobmoellersap Looking through some of the resources that you included, I feel it may be best to stray away from the Go workspaces since I don't think it quite solves the problem referenced in #2627. It also seems that it is likely our best route to go the direction of introducing the go.mod to the api sub-module to make it its own separate module. This ends up in the creation of a multi-module repository which while isn't absolute best practice, is acceptable when necessary. As mentioned in the Go wiki's "should I have multiple modules in the same repository?" section:
if you have a repository with a complex set of dependencies, but you have a client API with a smaller set of dependencies. In some cases, it might make sense to have an api or clientapi or similar directory with its own go.mod, or to separate out that clientapi into its own repository.
I think our use case aligns with this point and would warrant the creation of a multi-module repository. If we do this at the time of scaffolding we shouldn't have to do any weird VCS manipulation. I think we would fall under the first problem class mentioned in the Go wiki's "is it possible to add a module to a multi-module repository?" section:
The first class: the package to which the module is being added to is not in version control yet (a new package). This case is straightforward: add the package and the go.mod in the same commit, tag the commit, and push.
TLDR: My suggestion is we drop the Go workspaces approach and instead try the multi-module route that was originally suggested in #2627
@camilamacedo86 @jakobmoellersap What do you think?
@everettraven Agree with all of the above, except for
I think our use case aligns with this point and would warrant the creation of a multi-module repository. If we do this at the time of scaffolding we shouldn't have to do any weird VCS manipulation.
I think the first case, while straight-forward, is a bit misleading to read:
The first class: the package to which the module is being added to is not in version control yet (a new package). This case is straightforward: add the package and the go.mod in the same commit, tag the commit, and push.
This means that to make this work correctly we would still have to create a tag for the release of the module something like v1.0.0-alpha1 and push it to make the go.mod work. Before the git push, go mod tidy will not work since the VCS cannot be targeted and the go commands will throw an error trying to resolve the module.
So the workflow for multi-module creation would be:
kubebuilder initkubebuilder edit --multi-module-project=true=> this will lead to the go mod entry being created as a dependency.go mod tidy=> fails because the module is not available in VCS (even when trying to use areplace directive)git commit -am "kubebuilder multi-module project initialization"git tag api/v1alpha1/v1.0.0-alpha1git pushgo mod tidy=> now succeds, and can also be used together with replace statement
This is the same problem I encountered in https://github.com/kubernetes-sigs/kubebuilder/pull/2880#issuecomment-1228829539
What would be your suggestion to solve this @everettraven @camilamacedo86 ? If we do not interact with VCS here after creating the second module and depending on it, I think there is no way for kubebuilder to ensure project integrity (or that go mod tidy will run)
I think still that we should close this PR as I do not think workspaces are the way forward here. Question is how else are we gonna achieve this.
Hi @jakobmoellersap,
Regards your comment: https://github.com/kubernetes-sigs/kubebuilder/pull/2880#issuecomment-1231172728 Why that will fails? Why we need to scaffold the files without any api scaffold? Also, I do not think that we need to git add at all we just need to install the module inside of the api dir before running go mod tidy for the project.
Why not: Create a new plugin that enables it then when we call init|edit with plugins=multi-module/v1alpha we will:
- a) update the Dockerfile and track it in the Project file (such as it is done with all plugins)
- b) in this case this plugin must be tracked under the layout such as we do with the declarative one. see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v3-addon-and-grafana/PROJECT#L4
- c) check if has Resources created if not stop if yes, then scaffold the files on the api dir
Therefore, when we call the create api (we do not need to pass the multi-module/v1alpha ) the create api subcommand implemented for this plugin will be called in the chain always then we can:
- a) the templates under creating api should be configured with
f.IfExistsAction = machinery.SkipFileso that if they are created already we will not overwrite them. example: https://github.com/kubernetes-sigs/kubebuilder/blob/a44f85fab72ebbe5bb4f6dfbff9f12838c62a25b/pkg/plugins/golang/declarative/v1/scaffolds/internal/templates/channel.go#L44
WDYT?
c/c @everettraven
@camilamacedo86 I think I misunderstood you earlier and retried the approach manually.
The main question for me now is in the replace statement. Is it acceptable to have a replace directive like
require github.com/jakobmoellersap/someothertest.com/api/v1alpha1 v0.0.1
replace github.com/jakobmoellersap/someothertest.com/api/v1alpha1 => ./api/v1alpha1
in the main go.mod. This will allow the resolution to work but it will also mean that there can be a discrepancy between the "real" v0.0.1 and the one replaced locally. If i do not issue the replace statement, the module will not run.
If it is, then I think we can go ahead with a separate plugin as mentioned above
@jakobmoellersap @camilamacedo86 I think having it be a new plugin is reasonable. I think it would be okay to have that replace directive added to the main go.mod. I do wonder if we go that route if we should leave a comment above the replace directive mentioning why it is there and when it may be appropriate to comment it out (or remove it)
@everettraven @camilamacedo86 I created the new flow with a new Plugin here: https://github.com/kubernetes-sigs/kubebuilder/pull/2901,
please have a look if you find time.
Side-Note: I think we can take our learnings here and actually use a go.work file to create a local development context (which we put in .gitignore). Then we do not have to use the replace-statement. We could then use the proper build without go.work in the dockerfile but would have to put a disclaimer in the plugin that this won't work until the release tags are set properly. For now I will leave that out and use a simple replace in the go.mod
Closing here in favor of the next PR as this will surely not be merged.