Moving packages not intended for external usage to internal

[fabriziopandini]

This is a follow up of #5440 (comment) and #5440 (comment) and

User Story

As a user I would like to be have a clear signal about what is is subject to version guarantees and what not
As a developer I would like to be able to continually evolving CAPI and add new feautures, with clear boundaries between what can change and what instead should respect version guarantees

Detailed Description
For historical reason (kubebuilder scaffolding, merge of CABPK/KCP/CAPD into a single repository, etc) CAPI public surface is wider than we really want to, and this could lead to use case like the one described in the comment above whit users starting to rely on things that we are not committed to support long term.

This cause frustration both in users and maintainers.

This issue is about starting a code reorg activity for moving packages not intended for external usage to internal, thus avoiding confusions for people importing Cluster API as a go library and provide cleaner boundaries to the maintainers.

Anything else you would like to add:

The operation is intended to non have impact (or to have minimal impact to the users), so the proposal is to gate decision on what can be moved to internal by an analysis of Cluster API usage as a library in following providers implemented outside of the Cluster API repository:

• CAPA
• CAPG
• CAPT
• CAPV
• CAPZ

E.g for the bootstrap/kubeadm folder, following changes seems reasonable

bootstrap/kubeadm/controllers → bootstrap/kubeadm/internal/controllers (*)
bootstrap/kubeadm/types/upstreamv1beta2 → internal/kubeadm/types/upstreamv1beta2
bootstrap/kubeadm/types/upstreamv1beta3 → internal/kubeadm/types/upstreamv1beta3

(*) Even if we are moving the implementation detail of CABPK controllers to internal, it could make sense to provide a way for creating reconcilers, thus allowing users to combine CAPI controllers in a single binary if they want to.

/kind cleanup
/area code-organization

[fabriziopandini]

/hold
I'm going to propose this change to the community during Oct 20th office hours

[fabriziopandini]

/hold cancel

Overall +1 from the community with following notes

• [Jakob] pkg.go.dev provides "imported by" lists per package, we'll just have to go through all individual packages
• [Cecile] usage should not be the only criteria; if other projects are using CAPI packages improperly it should be brought to attention and alternative discussed

@CecileRobertMichon please confirm last point summarise your comment

[CecileRobertMichon]

+1, also this is the kind of change that should definitely not go in a patch release (but I think we've agreed to bug fixes only so we should be good on that front)

[vincepri]

+1

[sbueringer]

+1

[vincepri]

/milestone v1.1

[vincepri]

Let's make sure to look at the current API usage in the providers first before moving * to internal, we can go on a case-by-case basis

[fabriziopandini]

/help

I have kicked of this work with #5493 but there is definitely room for other people helping in this effort, which hopefully in most cases it is also more straight forward than the linked PR (move, adjust import without additional changes).
If someone want to help, it will be really useful to made a first pass on packages and propose a list of changes so than this issue can be labeled good-first-issue and the work federated.

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

I have kicked of this work with #5493 but there is definitely room for other people helping in this effort, which hopefully in most cases it is also more straight forward than this PR (move, adjust usage without additional changes).
If someone want to help, it will be really useful to made a first pass on packages and propose a list of changes so than this can work can be labeled good-first-issue and the work federated.

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.

[fabriziopandini]

Below a first set of task that should be an easy go for new contributors

The ask is to move controllers packages listed below to internal, then add on the original controllers package an alias.go file implementing a shim for reconcilers only, like the one in https://github.com/kubernetes-sigs/cluster-api/blob/main/bootstrap/kubeadm/controllers/alias.go

• [@shivanshu1333] /exp/controllers --> /exp/internal/controllers
• [@vibhorrawat] /exp/addons/controllers --> /exp/addons/internal/controllers
• [@kaitoii11] /test/infrastructure/docker/controllers --> /test/infrastructure/docker/internal/controllers
• /test/infrastructure/docker/exp/controllers --> /test/infrastructure/docker/exp/internal/controllers

In order to coordinate and fast track PRs, please add a comment on this issue if you are picking up one of this tasks, then open a separated PR for each of them

/good-first-issue

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Below a first set of task that should be an easy go for new contributors

The ask is to move controllers packages listed below to internal, then add on the original controllers package an alias.go file implementing a shim for reconcilers only, like the one in https://github.com/kubernetes-sigs/cluster-api/blob/main/bootstrap/kubeadm/controllers/alias.go

• /exp/controllers --> /exp/internal/controllers
• /exp/addons/controllers --> /exp/addons/internal/controllers
• /test/infrastructure/docker/controllers --> /test/infrastructure/docker/internal/controllers
• /test/infrastructure/docker/exp/controllers --> /test/infrastructure/docker/exp/internal/controllers

In order to coordinate and fast track PRs, please add a comment on this issue if you are picking up one of this tasks, then open a separated PR for each of them

/good-first-issue

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.

[shivanshuraj1333]

/assign

[shivanshuraj1333]

/unassign

[shivanshuraj1333]

I can pick up /exp/controllers --> /exp/internal/controllers :)

[vibhorrawat]

@fabriziopandini I would like to pick this one- /exp/addons/controllers --> /exp/addons/internal/controllers
Also, I'm a novice and would definitely need some pointers for the second half of the solution; which is then add on the original controllers package an alias.go file implementing a shim for reconcilers only
Let me know your thoughts. Thank you!

[sbueringer]

@vibhorrawat Great!

The idea is to add an alias.go like Fabrizio did for the kubeadm bootstrap controller in: https://github.com/kubernetes-sigs/cluster-api/pull/5493/files

alias.go is used to only expose the parts we want to be exposed. I think for /exp/addons/controllers it's fine to expose the Reconciler structs with their fields and the SetupWithManager funcs.

Feel free to ask, if you have any other questions.

[kaitoii11]

Hi
I would like to work on /test/infrastructure/docker/controllers --> /test/infrastructure/docker/internal/controllers!

[enxebre]

@fabriziopandini should we also review https://github.com/kubernetes-sigs/cluster-api/tree/main/util as part of this effort?

[fabriziopandini]

@enxebre depends on what do you have in mind with review, but ideally yes if we find out some packages not used outside CAPI we can consider to move them to internal. Not sure if we can get there given that my main concern are controllers.

[fabriziopandini]

@shivanshu1333 @vibhorrawat @kaitoii11 is there anything I can do to keep this effort moving?

[sbueringer]

@fabriziopandini @killianmuldoon WDYT should we follow a similar pattern for the new webhooks package? Would be relatively easy to change right now.

[killianmuldoon]

+1 for this where it makes sense. Even if there's nothing we're really exposing right now - I don't think there's much apart from the structs - it's good to have the pattern in place.

[vibhorrawat]

@fabriziopandini Hey, I will publish the PR during this week. Question about how do I test it locally? Kindly suggest. Thank you.

[fabriziopandini]

Question about how do I test it locally? Kindly suggest. Thank you.

you can integration and unit tests with make test, and then if you want you can also run E2E test locally. more info in https://cluster-api.sigs.k8s.io/developer/testing.html

[vibhorrawat]

@fabriziopandini Hi there, just an update from my end, I'm ready with the required changes for -
/exp/addons/controllers --> /exp/addons/internal/controllers
However, I'm yet to test the code changes locally. As of now, following the suggested testing page. Hopefully, I will have something to share before end of this week. Kindly bear with me. Thank you!

[sbueringer]

@vibhorrawat If you want, you can also open a WIP PR and CI will run all the tests for you. (just prefix the PR title with [WIP])

[vibhorrawat]

@sbueringer @fabriziopandini Submitted the PR for review with [WIP] prefix. Kindly have a look whenever convenient. Besides, I will continue my effort towards testing the code changes locally.

[kaitoii11]

If there aren't any takers, is it okay if I also work on /test/infrastructure/docker/exp/controllers --> /test/infrastructure/docker/exp/internal/controllers ?

[sbueringer]

@kaitoii11 Yup!

[fabriziopandini]

/close
the first iteration of this work is completed, we are going to open new issues as soon as new opportunity arises

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

/close
the first iteration of this work is completed, we are going to open new issues as soon as new opportunity arises

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.