[RS-23978] Make operator api a go module #3786
Conversation
LorcanMcVeigh
force-pushed
the
lorcan/create-submodule
branch
from
f084ed1 to
7648d83
Compare
LorcanMcVeigh
force-pushed
the
lorcan/create-submodule
branch
from
82ea1f9 to
94bdfe2
Compare
| @@ -15,6 +15,7 @@ | |||
| package v1 | |||
|
|
|||
| import ( | |||
| operatorapi "github.com/tigera/operator/api/v1" | |||
There was a problem hiding this comment.
nit: could we rename operatorapi to api? Maybe just v1? Not sure, but find operatorapi quite long when it's in the same repo...
There was a problem hiding this comment.
Yep - conventionally we have called this operator in other packages (sometimes operatorv1 but I think that is also more wordy than it needs to be).
v1 is slightly less good - there are several v1 packages used in this codebase.
There was a problem hiding this comment.
I actually thought the opposite. Having an import named operator in operator might have been confusing so I opted for something more descriptive but happy to change it if that's the consensus
There was a problem hiding this comment.
Yeah, it's just what we have used (somewhat inconsistently) in this repo.
| go 1.23.5 | ||
|
|
||
| require ( | ||
| github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring v0.80.1 |
There was a problem hiding this comment.
It would be nice to remove this dependency as well
There was a problem hiding this comment.
This import is used in the Endpoint API here https://github.com/tigera/operator/blob/master/api/v1/monitor_types.go#L69 for the Interval and ScrapeTimeout fields. In order to remove that dependency we would need to change the type of those fields or move the Endpoint struct to somewhere else. Moving the struct would require moving the ServiceMonitor, ExternalPrometheus & MonitorSpec out of the API as well because those structs are all dependent on each other in the installation_types.go .
I don't know how/where these definitions are used, so any guidance on this would be much appreciated
There was a problem hiding this comment.
Sounds like it might be more work than it is worth.
| const ( | ||
| Automatic AssignmentMode = "Automatic" | ||
| Manual AssignmentMode = "Manual" | ||
| Automatic operatorapi.AssignmentMode = "Automatic" |
There was a problem hiding this comment.
These should be defined in the operator API package rather than here.
There was a problem hiding this comment.
There is an overlap in var name with PolicyMode https://github.com/tigera/operator/blob/master/api/v1/installation_types.go#L228 so unless we want to rename one of the vars.
Given I don't work on this code or with these vars at all. I would probably favour someone with more experience with them to rename them. Thoughts?
There was a problem hiding this comment.
We should rename both of these constants - the convention for enums is that they should include the type in the name to avoid this type of conflict.
type AssignmentMode string
const (
AssignmentModeAutomatic = "Automatic"
AssignmentModeManual = "Manual"
)| @@ -658,6 +656,8 @@ func (nt NATOutgoingType) String() string { | |||
|
|
|||
| const NodeSelectorDefault string = "all()" | |||
|
|
|||
| type AssignmentMode string | |||
There was a problem hiding this comment.
Missing the const definitions for this type?
Description
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.