-
Notifications
You must be signed in to change notification settings - Fork 73
feat(crd): allow child resources to get additional labels and annotations #363
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
Conversation
Signed-off-by: Soumya Ghosh Dastidar <[email protected]>
@Abhra303 opened a PR directly instead of creating an issue. I know there's another PR #362 that adds something similar but it inherits the values to both the pod and sts. In our usecase we just want the tracking annotations/labels to be passed down to the main child resources managed by the CRD and not to the pods itself. |
Hi! I personally like this PR's idea better than #362 (mine 😅). However, it looks like it is still lacking some tests. If you need, you can copy/adapt them from my PR 😉 |
Thanks for the pointers @Pinimo ! Will add some tests! |
Signed-off-by: Soumya Ghosh Dastidar <[email protected]>
d4161d0
to
3fd9303
Compare
On second thought, I think you are right to have a field for this. But in general, I prefer a clear and specific field for this. E.g. spec:
resourcesMetadata:
labels: ...
annotations: ... I don't think it is common to pass labels/annotations down to its owned resources directly from CRD's metadata. What do you think? |
I have seen some operators pass down the labels and annotations on the parent resource specially if there's not concept of I would have gone for a separate field but i think it'll just confuse people with the existing Spec.labels applied to pods directly. Which is why I had the original field to optionally inherit the labels which makes it an explicit user choice to reuse the parents metadata for all child resources and not just the sts. Unless we want to introduce breaking change and have a separate spec for pod and sts like we have serviceSpec I would suggest reverting the last commit to have the optional flag to inherit and let users choose the behaviour for all direct child resources and they can specify any specific metadata for pods/service inside the existing spec fields. |
Honestly, the existing For example: metadata:
labels:
operator: "dragonfly"
crd-group: db
spec:
ownedObjectsMetadata:
labels:
operator: "dragonfly"
... Also the reconciler listens to spec changes only. So if you change the metadata of CRD, the reconciler won't be triggered and owned resources will not have the updated metadata labels. |
Okay then I will go ahead and add this field,
For the existing spec.Labels I will leave it as is, I don't want to stretch the scope of this PR so maybe a followup to change it to spec.podSpec.labels and deprecating old field will be better |
4c3e452
to
0f913c1
Compare
@Abhra303 updated |
Signed-off-by: Soumya Ghosh Dastidar <[email protected]>
0f913c1
to
173e6ca
Compare
Thanks! |
@Abhra303 seems like the CI started failing. it's failing on main too https://github.com/dragonflydb/dragonfly-operator/actions/runs/17944604330/job/51027928621 Don't think the failure is related to my changes. Can you please take a look |
Yea, it is not related to your change. I'll look. |
@gdsoumya, now the test is timing out, can you please check? |
Signed-off-by: Soumya Ghosh Dastidar <[email protected]>
Pushed fix, hopefully CI will pass now |
@Abhra303 it passed! |
Thanks! |
@Abhra303 any eta on a new release with this change? |
This PR adds an optional flag in the CRD to pass down the metadata(labels and annotations) of the CR to the child resources. This allows for any custom tracking labels/annots to be passed down to the child resources