Skip to content
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

[chore] move controllers under internal #3633

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jan 20, 2025

Description:
Move controllers under internal so their API is not exported.

@atoulme atoulme requested a review from a team as a code owner January 20, 2025 00:52
@atoulme atoulme force-pushed the move_controllers branch 2 times, most recently from 072aa2e to a4c1b01 Compare January 20, 2025 01:29
@swiatekm
Copy link
Contributor

This placement of controllers is a kubebuilder standard. I'm not sure how much that matters to us at this point, but it would certainly make the migration to kubebuilder v4 more difficult. WDYT @open-telemetry/operator-approvers ?

@jaronoff97
Copy link
Contributor

Yes, i think we should do the kubebuilder v4 migration first as that unblocks a few other issues IIRC.

@atoulme
Copy link
Contributor Author

atoulme commented Jan 21, 2025

I don't see an issue for moving to kubebuilder 4, is it ok to open one? Is anyone working on it?

@frzifus
Copy link
Member

frzifus commented Jan 23, 2025

I dont see a huge downside of exposing it or benefit of moving it into internal here. Thats why I would prefer to just follow whatever kubebuilder standard is.

@atoulme
Copy link
Contributor Author

atoulme commented Jan 23, 2025

The problem is that you're currently exposing incomplete APIs. It is possible other downstream projects may start to depend on it and bind you to a specific go API, which doesn't seem to be the intent of this project. See
https://pkg.go.dev/github.com/open-telemetry/[email protected]/controllers

@atoulme
Copy link
Contributor Author

atoulme commented Jan 23, 2025

I am reading through kubebuilder resources; I see a reference to controllers in the kubebuilder book, but it's placed under internal:
https://book.kubebuilder.io/cronjob-tutorial/controller-implementation

https://github.com/kubernetes-sigs/kubebuilder/blob/master/docs/book/src/cronjob-tutorial/testdata/project/internal/controller/cronjob_controller.go

Is there a reference elsewhere to placing controllers under top level folder?

@swiatekm
Copy link
Contributor

It used to be at top level in v2: https://book-v2.book.kubebuilder.io/cronjob-tutorial/controller-implementation.

I guess if it's under internal in subsequent versions, then moving it right now won't really make the migration any more difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants