Skip to content

Move apis package to a sub-module#5077

Open
iypetrov wants to merge 11 commits into
open-telemetry:mainfrom
iypetrov:move-apis-package-to-a-sub-module
Open

Move apis package to a sub-module#5077
iypetrov wants to merge 11 commits into
open-telemetry:mainfrom
iypetrov:move-apis-package-to-a-sub-module

Conversation

@iypetrov
Copy link
Copy Markdown
Contributor

Description:
This is a continuation of PR #4989. The apis package is moved to a dedicated sub-module.

Link to tracking Issue(s):

Testing:
No new tests are added, just adapt some of the existing ones

@iypetrov iypetrov requested a review from a team as a code owner May 13, 2026 14:32
@iypetrov
Copy link
Copy Markdown
Contributor Author

hey @swiatekm, quick question about apis/v1alpha1/convert.go - right now OpenTelemetryCollector implements the Convertible interface from sigs.k8s.io/controller-runtime/pkg/conversion. This creates a direct dependency on controller-runtime in the API submodule, which could be problematic for users who only want to consume the opentelemetry-operator CRDs without pulling in the full controller-runtime stack. Would it be possible to decouple this? For example, by moving the conversion logic into an internal package or refactoring it into standalone helper functions (which would be my preference)? wdyt?

@iypetrov iypetrov force-pushed the move-apis-package-to-a-sub-module branch 2 times, most recently from 304400c to 00ae37a Compare May 13, 2026 14:56
@swiatekm
Copy link
Copy Markdown
Contributor

hey @swiatekm, quick question about apis/v1alpha1/convert.go - right now OpenTelemetryCollector implements the Convertible interface from sigs.k8s.io/controller-runtime/pkg/conversion. This creates a direct dependency on controller-runtime in the API submodule, which could be problematic for users who only want to consume the opentelemetry-operator CRDs without pulling in the full controller-runtime stack. Would it be possible to decouple this? For example, by moving the conversion logic into an internal package or refactoring it into standalone helper functions (which would be my preference)? wdyt?

I'm fine moving or refactoring it. We're going to be dropping v1alpha1 Collectors soon either way.

iypetrov added 8 commits May 13, 2026 21:37
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
…to internal package

Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
@iypetrov iypetrov force-pushed the move-apis-package-to-a-sub-module branch from 215e391 to 3c908e2 Compare May 13, 2026 18:38
iypetrov added 2 commits May 13, 2026 21:46
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
@iypetrov
Copy link
Copy Markdown
Contributor Author

I'm fine moving or refactoring it. We're going to be dropping v1alpha1 Collectors soon either way.

@swiatekm I moved the conversion functions into helper functions and now the OpenTelemetryCollector doesn't implement the Convertible interface.

@swiatekm
Copy link
Copy Markdown
Contributor

swiatekm commented May 14, 2026

I'm fine moving or refactoring it. We're going to be dropping v1alpha1 Collectors soon either way.

@swiatekm I moved the conversion functions into helper functions and now the OpenTelemetryCollector doesn't implement the Convertible interface.

Looks pretty good to me. I think we can even move the conversion code completely into internal/webhook/conversion.go. There isn't any good reason for it to live in the api module. WDYT @open-telemetry/operator-approvers ?

@iypetrov
Copy link
Copy Markdown
Contributor Author

Looks pretty good to me. I think we can even move the conversion code completely into internal/webhook/conversion.go. There isn't any good reason for it to live in the api module. WDYT @open-telemetry/operator-approvers ?

I'm okay with it, if others are okay with it will commit the change.

Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

E2E Test Results

 38 files  261 suites   2h 20m 12s ⏱️
110 tests 109 ✅ 1 💤 0 ❌
283 runs  279 ✅ 4 💤 0 ❌

Results for commit 4232c2e.

♻️ This comment has been updated with latest results.

@iypetrov
Copy link
Copy Markdown
Contributor Author

Looks pretty good to me. I think we can even move the conversion code completely into internal/webhook/conversion.go. There isn't any good reason for it to live in the api module. WDYT @open-telemetry/operator-approvers ?

P.S. I already moved it. If v1alpha1 is going to be removed anyway, I think it’s fine to merge this beforehand.

Copy link
Copy Markdown
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. I think we're also going to need dependabot config updates so we can keep the shared dependencies synchronized, but that can happen in a follow-up.

Since this is a major change, I'm going to need more approvals here, with at least one more maintainer approval. @open-telemetry/operator-approvers if you're opposed to this, this is your last chance to speak up.

@swiatekm swiatekm requested review from a team May 15, 2026 13:48
@iypetrov
Copy link
Copy Markdown
Contributor Author

The changes look good to me. I think we're also going to need dependabot config updates so we can keep the shared dependencies synchronized, but that can happen in a follow-up.

Since this is a major change, I'm going to need more approvals here, with at least one more maintainer approval. @open-telemetry/operator-approvers if you're opposed to this, this is your last chance to speak up.

Is there some example dependabot config for such case?

@swiatekm
Copy link
Copy Markdown
Contributor

swiatekm commented May 15, 2026

The changes look good to me. I think we're also going to need dependabot config updates so we can keep the shared dependencies synchronized, but that can happen in a follow-up.
Since this is a major change, I'm going to need more approvals here, with at least one more maintainer approval. @open-telemetry/operator-approvers if you're opposed to this, this is your last chance to speak up.

Is there some example dependabot config for such case?

I'm honestly not sure if it supports this very well. If not, we'll move Go dependency updates to renovate, which I'm confident is capable of it. We'd wanted to do this for a while anyway and just never got around to it.

The api package specifically has few enough dependencies that it's not going to be a big deal even if we need to do some manual syncing until we figure it out.

Copy link
Copy Markdown
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also move the v1alpha1 to keep all the APIs in one place? the v1alpha1 hosts more than just the collector.

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.

3 participants