-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Importing API types pulls in lots of dependencies #9011
Comments
/triage accepted |
This all sounds good, a few additional comments would be:
Thoughts? |
Agreed, I don't have context on why If they don't need to be versioned, having them centrally as for
Ack yes, I can make a PR for this separately so it's considered as a different part of the same effort I think it would be good to have a consistent approach across CAPI and the related repos, if there is a pattern we can come up with that we can document then I would like to also pursue updating the provider APIs to mitigate this issue there as well |
The reason why Cluster and ClusterClass are separate is the following:
I agree that we can just follow the same pattern for all webhooks.
The validate / default funcs / webhooks don't have to be versioned. Whenever we introduced a new apiVersion we moved the methods from the previous apiVersion to the new one :). So overall sounds good to me. I'm absolutely in favor! |
+1 Agree anything we can do to limit importing |
+1, thanks for creating this Joel. I find having the ability to vendor the API cleanly critical for consumers and to ease adoption of the project. |
Just to double-check. I think I might have brought this up at the time. If I now take a look at our current code this doesn't seem to be an issue as as we only depend on "sigs.k8s.io/controller-runtime/pkg/conversion" (for conversion.Hub) in old apiVersions. So it won't be a problem for cleaning up dependencies of the latest apiVersion (which I assume is our main goal). |
+1 from my side, thanks for reviving this discussion @JoelSpeed! |
I did a bit of work on the code for this today and came across a small issue. Currently the package I can see 3 possible paths forward:
Any preferred method from those or alternative suggestions from anyone? |
From a quick look it should be relatively easy to move the defaulting to the tests themselves rather than the builders. There's only a few instances of this usage. @chrischdi - I think you added this when moving these tests to envtest, is there something I'm not thinking of that could stand in the way? |
Going to play with this idea then, will report back if I hit more issues. Expect some PRs over the coming days |
If I got it right we already had and solved this issue before. We have a separate test package here for those kind of tests: https://github.com/kubernetes-sigs/cluster-api/tree/3e16abc3df71314bbaee770846beba45c0eb170d/internal/webhooks/test EDIT: My bad. That's a separate issue, but maybe you'll hit that one as well :). Agree with Killian for your current problem |
Nope, looks good. At the time of implementing it was just the most straightforward one. Should be totally fine to default at the test and remove that from the builder 👍 @sbueringer because we just chatted about the |
I've just posted two PRs to show the progress I've made so far on the main Cluster v1beta1 APIs, the first of which for scheme builder should be ready for review, the second I think is ready for review apart from I know I have to sort the MachineDeployment defaulting in tests, otherwise I think that should be fairly good to go |
Both PRs are now ready for review and, combined, clear the deps for the main APIs (Machine, MS, MD, Cluster etc). Will start to look at the exp APIs next |
Have added a TODO list with my initial thoughts on scope here, any additional points we need for this issue? |
Seems ~ good to me. Does your list cover all those APIs? stable exp clusterctl test Totally fine to focus on stable + exp + maybe clusterctl (because it's a quickwin) and then create a follow-up issue for test (I would mostly migrate test as well for consistency across the entire code base) P.S. I'll try to find some time to review your PRs soon. Pretty low bandwidth at the moment |
I've updated the list to be very explicit, will keep track of the PRs in the list as we go along as well |
@killianmuldoon Do you have some time to replicate this issue for CAPV and work on it going forward? (implementation is not urgent, won't make it / doesn't have to make it into CAPV v1.8) |
By that you mean removing the controller-runtime package from the CAPV API packages? |
Basically implementing the same in CAPV that Joel is implementing in core CAPI. So yes, if I got your question correctly :) |
Sure thing - I'll create an issue and assign it to myself. |
Sounds good. Sorry didn't see it |
Will pick |
Let's do a final round at the end to add import restrictions everywhere. I forgot to ask for them at least a few times and we don't have them in the packages we added earlier. Should be good enough to just add them once we moved everything. |
Will pick |
Also picking |
Thank you very much! |
Thx everyone so far. As far as I can tell we're now down to Ankita's enforce import restriction PR (#9461) and docs |
Nice work everyone!! As far as I can tell we should be done - apart from documentation. @JoelSpeed Would be good if you can maybe do a final verification if the transitive dependencies look good from your side (in case you didn't do it already) |
Based on JoelSpeed/capi-api-import-test@8800fd3 we are doing pretty well across the board, however, the Addons API, which I added separately, JoelSpeed/capi-api-import-test@8c6aa3d, adds a bunch of deps including controller runtime back in. I see the import restrictions are in place for the addons, I guess it doesn't work transitively? |
Seems we are importing |
Yeah - looks like we need a refactor of some of that code - I'll have a quick go and see if it's straightforward. |
If it is only for one function, let's consider duplicating it |
Added a new sub task:
|
@sbueringer @enxebre should we close this issue? the development work is completed and we have our code as a documentation/example for how to do the same in providers (if they want to) |
I think we have to implement " Add verification to avoid regressions" otherwise we have 0 protection against transitive dependencies sneaking in. Which can happen very easily. One idea how this can be done was suggested here: #9482 (comment) (same PR shows how easy it is to get transitive dependencies) |
/priority important-longterm |
What steps did you take and what happened?
Create a simple program, as below and run
go mod tidy
:This results in a go.mod with 56 dependencies. Included in this list is
controller-runtime
which, is also imported by a lot of consumers of the API types.controller-runtime
has evolved a lot over time and, because it is being imported by the APIs currently, means that any project consuming the API types must also import the same controller-runtime as the rest of CAPI.There are use cases where, building a consumer, I do not need to use any of the code (defaulting, validation etc) from CAPI and as such, do not need such a restriction on my
controller-runtime
version.If we modify the way the
SchemeBuilder
works and move the webhooks to a separate package, then we can reduce the required imports for the API package down to just 31 dependencies, and, these dependencies do not includecontroller-runtime
or other fast moving/changing packages. There are still obviously a good number of deps in there from K8s paths, but these are mostly API machinery related and have pretty stable APIs.I think if we can separate the webhooks out in this way (I notice Cluster and ClusterClass already are?) then I think this would make consuming these API types in other projects a lot easier.
Note how
k8s.io/api
does not include much code with the API types for this reason as well.The alternative solution for consumers is to copy/paste the types out to their own repos which is less ideal.
What did you expect to happen?
API types should not be tied to many dependencies, especially not controller-runtime.
Cluster API version
main branch
Kubernetes version
N/A
Anything else you would like to add?
No response
Label(s) to be applied
/kind bug
One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels.
TODO
./api
(🌱 Remove reliance on controller-runtime scheme builder #9045)./controlplane/kubeadm/api
(🌱 Remove reliance on controller-runtime scheme builder for remaining API groups #9266)./bootstrap/kubeadm/api
(🌱 Remove reliance on controller-runtime scheme builder for remaining API groups #9266)./exp/api
(🌱 Remove reliance on controller-runtime scheme builder for experimental APIs #9185)./exp/addons/api
(🌱 Remove reliance on controller-runtime scheme builder for experimental APIs #9185)./exp/ipam/api
(🌱 Remove reliance on controller-runtime scheme builder for experimental APIs #9185)./exp/runtime/api
(🌱 Remove reliance on controller-runtime scheme builder for experimental APIs #9185)./cmd/clusterctl/api
(🌱 Remove reliance on controller-runtime scheme builder for remaining API groups #9266)./test/infrastructure/docker/api
(🌱 Remove reliance on controller-runtime scheme builder for remaining API groups #9266)./test/infrastructure/docker/exp/api
(🌱 Remove reliance on controller-runtime scheme builder for remaining API groups #9266)./test/infrastructure/inmemory/api
(🌱 Remove reliance on controller-runtime scheme builder for remaining API groups #9266)./api
(🌱 Move API v1beta1 webhooks to a separate package #9047)./controlplane/kubeadm/api
(🌱 Move Kubeadm API v1beta1 webhooks to separate package #9410)./bootstrap/kubeadm/api
(🌱 Move Kubeadm API v1beta1 webhooks to separate package #9410)./exp/api
(🌱 Move experimental API v1beta1 webhooks to separate package #9417)./exp/addons/api
(🌱 Move experimental addons API v1beta1 webhooks to separate package #9438)[ ] Move webhooks for(N/A)./exp/ipam/api
[ ] Move webhooks for(N/A)./exp/runtime/api
[ ] Move webhooks for(N/A)./cmd/clusterctl/api
./test/infrastructure/docker/api
(🌱 Move docker infrastructure API v1beta1 webhooks to separate package #9458)./test/infrastructure/docker/exp/api
(🌱 Move docker infrastructure experimental API v1beta1 webhooks to separate package #9460)./test/infrastructure/inmemory/api
(🌱 Move inmemory infrastructure API v1beta1 webhooks to separate package #9459)./api
🌱 Add verify-import-restrictions to enforce import restrictions #9407The text was updated successfully, but these errors were encountered: