Create a go module for the api package #37266
Conversation
|
Commit 682ae69 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
maintainer-s-little-helper
bot
added
dont-merge/needs-sign-off
github-actions
bot
added
the
kind/community-contribution
alvaroaleman
force-pushed
the
submodule
branch
from
682ae69 to
e295221
Compare
|
Commit 682ae69 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
alvaroaleman
force-pushed
the
submodule
branch
from
e295221 to
6be0824
Compare
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-sign-off
If someone imports the Cilium API types, they currently endup with a transient dependency to everything Cilium depends on, which is a lot. In order to avoid this, this change creates a go module for the api. That doesn't yet fully solve the problem, because that go module still depends on the main cilium module. We can clean that up in a follow-up as well as adding a check to forbid adding that. After this change is merged, anytime we tag a new release we have to make sure that the same git commit gets tagged for the main module as it already happens today, as well as for the submodule in the form ` pkg/k8s/apis/cilium.io/v2/v1.2.3`, ref[0]. I am happy to look into this if someone points me to the release tooling. Ref cilium#36265 but I'd like to keep the issue open until any dependency from the api module to the main module is cleaned up. [0]: https://go.dev/wiki/Modules#faqs--multi-module-repositories Signed-off-by: Alvaro Aleman <alvaroaleman@users.noreply.github.com>
alvaroaleman
force-pushed
the
submodule
branch
from
6be0824 to
725c1df
Compare
aanm
added
the
dont-merge/discussion
|
I totally support this effort! Does it make sense to use go workspaces? |
|
The policy tree is probably going to be the biggest cause of problems here:
Additionally, we should consider splitting the API types and internal types completely; there's no reason for an api type to have unexported bookkeeping fields. So, we need to do a few things:
|
There's some discussion about workspaces in the link below which highlights some drawbacks. I'm not sure if those drawbacks are painful enough to cause us to choose a different solution (as I don't fully grasp the consequences of the alternative), but it might shed some light: |
Reading the workspaces wiki, it explicitly calls out the client API as a good use for workspaces :-).
|
There was a problem hiding this comment.
Thanks for bringing this discussion forward @alvaroaleman! I'm sorry I couldn't join the community meeting last Wednesday. Nonetheless, I'll provide my thoughts in written form below.
In principle, I (and the Hubble team at large) am very much in favor of creating a separate module for the api package for all the reasons you mentioned and more (for instance, importing cilium in your project requires keeping track of all the replace directives which is tedious and prone to creating issues if not done; having a separate module without any replace directive would remove that hurdle).
However, we need to take careful considerations before actually doing that. I'll list below some of the things that I think must be taken into consideration:
- How do we pull off this change without breaking developers workflows? The answer could be to use replace directives from the main
go.modto the api's one or to push ago.workfile to the tree to leverage Go workspaces). - How do we avoid breaking downstream projects which rely on the API package? When creating the module (or modules) we may want to re-organize/re-group the various APIs we have under
api/v1. - What would need to be updated in the release process? For instance, we would need to tag
v1.17.0andapi/v1.17.0at the time of releasing v1.17.0), potentially remove replace directives (see comment below), etc. - How do we ensure that the API module does NOT depend on other Cilium packages? That's potentially a hard one to address but it's in my opinion almost pointless to create a separate module if it ends up requiring Cilium as a dependency.
- Do we want a single
apimodule or should we actually create different ones for the protobuf APIs and the Cilium API client? - We need to update Renovate and any process that takes care of updates and Renovate is known to not always work well with Go workspaces and projects with multiple modules (as pointed out by Chance here).
This list may not be exhaustive but I hope it can serve as a good starting points for things to consider. I remember we (the Hubble team) considered doing this at some point but never actually implemented the change. While I don't recall exactly why, the fact that there are many things to consider and address is probably part of the reason.
| @@ -0,0 +1,130 @@ | |||
| module github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2 | |||
|
|
|||
| replace github.com/cilium/cilium => ../../../../../ | |||
There was a problem hiding this comment.
Whenever we tag a release, these replace directives would need to be removed and the cilium version below be updated to match the version about to be tagged. Failing to do so means it ill be impossible to import the go module individually which makes splitting into a separate module effectively useless.
While this is something that can be done, we also need to consider that one cannot import the module unless it matches a tagged version (so there's no importing that un-tagged version that may fix a bug you're facing for instance).
As a point of reference, this is a process we follow for the cilium/fake project, which consists of 3 modules with interdependencies. A release PR (example) contains 2 commits: the first one removes any replace directive and changes the version of the modules to the one that will be tagged (that's the commit we then tag) and a follow-up commit that adds back the replace directives so that development workflows aren't broken.
If someone imports the Cilium API types, they currently end up with a transient dependency to everything Cilium depends on, which is a lot. In order to avoid this, this change creates a go module for the api. That doesn't yet fully solve the problem, because that go module still depends on the main cilium module. We can clean that up in a follow-up as well as adding a check to forbid adding that.
After this change is merged, anytime we tag a new release we have to make sure that the same git commit gets tagged for the main module as it already happens today, as well as for the submodule in the form
pkg/k8s/apis/cilium.io/v2/v1.2.3, ref. I am happy to look into this if someone points me to the release tooling.Ref #36265 but I'd like to keep the issue open until any dependency from the api module to the main module is cleaned up.
cc @joestringer
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.