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 TargetAllocator CRD to v1alpha1 #2904

Closed

Conversation

swiatekm
Copy link
Contributor

@swiatekm swiatekm commented Apr 27, 2024

Move the TargetAllocator CRD to v1alpha1. Since the CRD has a bunch of definitions in common with v1beta1.OpenTelemetryCollector, I've moved those to a common package. In the process, I've also gotten rid off some unnecessary conversions between v1beta1 and v1alpha1 enums.

Right now, this also includes changes from #2883. That should be merged first, then I'll rebase.

Supersedes #2894.

.chloggen/fix_load-initial-servicemonitors.yaml Outdated Show resolved Hide resolved
package v1beta1
// +kubebuilder:object:generate=true
// +groupName=opentelemetry.io
package common
Copy link
Member

Choose a reason for hiding this comment

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

I am concerned about using a common package for multiple versions. I haven't seen this being the case in other projects or k8s. All symbols are usually clearly versioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it help to alias them? I don't really want to copy all the shared structs and then define pointless conversions between them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could also make this package private under internal/?

Copy link
Member

Choose a reason for hiding this comment

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

internal could work, but I thought that the symbols will be exposed in the CRD

@swiatekm swiatekm force-pushed the chore/move-ta-crd-v1alpha1 branch from 8ab25dd to b536c53 Compare May 1, 2024 12:49
@swiatekm
Copy link
Contributor Author

swiatekm commented May 1, 2024

@pavolloffay this is the closest I can get to having mostly versioned symbols. The common structs are part of an internal package. I've aliased the enums, but for more complex types this causes more problems than it solves. The alternative would be to copy the shared structs to v1alpha1, which I don't really like the idea of.

Comment on lines +22 to +27
// TargetAllocatorPrometheusCR configures Prometheus CustomResource handling in the Target Allocator.
TargetAllocatorPrometheusCR common.TargetAllocatorPrometheusCR
// TargetAllocatorAllocationStrategy represent a strategy Target Allocator uses to distribute targets to each collector.
TargetAllocatorAllocationStrategy common.TargetAllocatorAllocationStrategy
// TargetAllocatorFilterStrategy represent a filtering strategy for targets before they are assigned to collectors.
TargetAllocatorFilterStrategy common.TargetAllocatorFilterStrategy
Copy link
Contributor

Choose a reason for hiding this comment

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

should this remain here? Or is this because we need to maintain a v1alpha1 and v1beta1 for this now?

@jaronoff97
Copy link
Contributor

I had one question about the TA_types.go file. My only gripe with this is referencing the internal package from files that we do publicly expose. Is it not possible to just reference the v1beta1 type from the v1alpha1? It looks like that's what prometheus operator may do?

@swiatekm
Copy link
Contributor Author

swiatekm commented May 1, 2024

I had one question about the TA_types.go file. My only gripe with this is referencing the internal package from files that we do publicly expose. Is it not possible to just reference the v1beta1 type from the v1alpha1? It looks like that's what prometheus operator may do?

If we're ok with that kind of pattern, that is putting shared structs in the latest version they appear in, then I think that should work?

@swiatekm
Copy link
Contributor Author

Closing in favor of #2918

@swiatekm swiatekm closed this May 22, 2024
@swiatekm swiatekm deleted the chore/move-ta-crd-v1alpha1 branch May 26, 2024 13:39
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