Add support for annotations on Deployment/StatefulSet resources in DruidNodeSpec #145
Conversation
AdheipSingh
requested review from
itamar-marom,
AdheipSingh and
cyril-corbon
Done ✅ Any strong preference on how these are merged? In my opinion it makes sense for the finer-grained DruidNodeSpec-level annotations to override the DruidSpec-level annotations (see implementation), but I see that we do the opposite with PodLabels. |
SamWheating
force-pushed
the
sw-add-annotations-on-sts
branch
from
f5e647e
to
ee37b18
Compare
|
@SamWheating does Reddit use the operator? if so, can we add you as users in the ADOPTERS.md ? |
Not in any official capacity, this work was completed on my own time and mostly just out of personal interest. |
There was a problem hiding this comment.
Hi @SamWheating just fix the function name and I'll approve
controllers/druid/handler.go
Outdated
| @@ -1287,6 +1289,22 @@ func makeLabelsForNodeSpec(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid, | |||
| return labels | |||
| } | |||
|
|
|||
| // makeAnnotationsForReplicationController returns the annotations for a Deployment or StatefulSet | |||
| // If a given key is set in both the DruidSpec and DruidNodeSpec, the node-scoped value will take precedence. | |||
| func makeAnnotationsForReplicationController(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) map[string]string { | |||
There was a problem hiding this comment.
| func makeAnnotationsForReplicationController(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) map[string]string { | |
| func makeAnnotationsForWorkload(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) map[string]string { |
There was a problem hiding this comment.
Ah whoops, I totally missed this 🤦 thanks for spotting!
controllers/druid/handler.go
Outdated
| @@ -1287,6 +1289,22 @@ func makeLabelsForNodeSpec(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid, | |||
| return labels | |||
| } | |||
|
|
|||
| // makeAnnotationsForReplicationController returns the annotations for a Deployment or StatefulSet | |||
There was a problem hiding this comment.
``
| // makeAnnotationsForReplicationController returns the annotations for a Deployment or StatefulSet | |
| // makeAnnotationsForWorkload returns the annotations for a Deployment or StatefulSet |
controllers/druid/handler.go
Outdated
| Namespace: m.Namespace, | ||
| Labels: ls, | ||
| Name: fmt.Sprintf("%s", nodeSpecUniqueStr), | ||
| Annotations: makeAnnotationsForReplicationController(nodeSpec, m), |
There was a problem hiding this comment.
| Annotations: makeAnnotationsForReplicationController(nodeSpec, m), | |
| Annotations: makeAnnotationsForWorkload(nodeSpec, m), |
controllers/druid/handler.go
Outdated
| Namespace: m.Namespace, | ||
| Labels: ls, | ||
| Name: fmt.Sprintf("%s", nodeSpecUniqueStr), | ||
| Annotations: makeAnnotationsForReplicationController(nodeSpec, m), |
There was a problem hiding this comment.
| Annotations: makeAnnotationsForReplicationController(nodeSpec, m), | |
| Annotations: makeAnnotationsForWorkload(nodeSpec, m), |
Fixes #142
Description
Adding a
ReplicationControllerAnnotationsfield to the DruidNodeSpec schema. This allows users to specify annotations to be applied to the StatefulSet / Deployment object.I'm not 100% sure of the naming - what do you think? I didn't want to call it
StatefulSetAnnotationssince the user can opt for a deployment instead.Also regenerated the API docs, which led to a huge diff as it appears they weren't regenerated after #53 (hence the huge diff)
This PR has:
I haven't tested this on a real kube cluster, but all of the e2e tests run. Is that sufficient for an overall minor/straightforward change?