feat(otel-allocator): use type for AllocationStrategy #1220
Conversation
| "github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/diff" | ||
| "github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/target" | ||
| ) | ||
|
|
||
| var _ Allocator = &consistentHashingAllocator{} | ||
|
|
||
| const consistentHashingStrategyName = "consistent-hashing" | ||
| const consistentHashingStrategyName = string(v1alpha1.OpenTelemetryTargetAllocatorAllocationStrategyConsistentHashing) |
There was a problem hiding this comment.
question: are we okay having the allocator import this from the operator? It means that the allocator now has a required dependency on the operator which i'm not sure we want. cc @pavolloffay
There was a problem hiding this comment.
I think it is fine here, as mentioned in the description we are not adding any additional dependencies.
The benefit would be that we have source of truth regarding the available options.
|
I have deployed operator and allocator and tried all 3 different setting for |
cmd/otel-allocator/Dockerfile
Outdated
| # Copy go mod and sum files | ||
| COPY go.mod go.sum ./ | ||
| # copy operator and allocator code | ||
| COPY . . |
There was a problem hiding this comment.
Can this change be restructured/avoided? Doing this can cause code changes that do not change dependencies to invalidate the cached FS layer with dependencies already downloaded, which can make developing/testing with this Dockerfile annoying.
There was a problem hiding this comment.
go mod download will fail we not copy the code first because of the new replacement directive
Alternatively we have to drop dependency to the operator here again.
There was a problem hiding this comment.
IMO we should drop the dependency on the operator, I think it's going to add more complexity than it's worth.
This reverts commit 6b9a54c.
# Conflicts: # apis/v1alpha1/opentelemetrycollector_types.go
…#1220) * feat(otel-allocator): use type for AllocationStrategy * ci(otel-allocator): add operator to build context of allocator * chore(otel-allocator): fix linting * Revert "ci(otel-allocator): add operator to build context of allocator" This reverts commit 6b9a54c. * feat(otel-allocator): rollback dependency to operator
Fixes #1101
This PR changes the AllocationStrategy from String to a new type.
Further it now the Allocator references now to the constants provided by the operator package.
Based on the changes of
go.modandgo.sum, the footprint of this additionally dependency seems to be minimal.