[chore] Move TargetAllocator CRD to v1alpha1 #2904
Conversation
apis/common/common.go
Outdated
| package v1beta1 | ||
| // +kubebuilder:object:generate=true | ||
| // +groupName=opentelemetry.io | ||
| package common |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Would it help to alias them? I don't really want to copy all the shared structs and then define pointless conversions between them.
There was a problem hiding this comment.
Could also make this package private under internal/?
There was a problem hiding this comment.
internal could work, but I thought that the symbols will be exposed in the CRD
swiatekm-sumo
force-pushed
the
chore/move-ta-crd-v1alpha1
branch
from
8ab25dd
to
b536c53
Compare
|
@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 |
| // 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 |
There was a problem hiding this comment.
should this remain here? Or is this because we need to maintain a v1alpha1 and v1beta1 for this now?
|
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? |
|
Closing in favor of #2918 |
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
commonpackage. 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.