-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: Add labels and annotations to pods. #681
feat: Add labels and annotations to pods. #681
Conversation
I'll give this and your other PRs a test tomorrow. |
e103f91
to
6329db4
Compare
Hey @cpitstick-latai this is a great start! I was able to easily get the annotations/labels working locally after running
helm upgrade --install open-feature-operator /home/todd/git/open-feature-operator/charts/open-feature-operator-v0.6.1.tgz
Release "open-feature-operator" does not exist. Installing it now.
Error: YAML parse error on open-feature-operator/templates/apps_v1_deployment_open-feature-operator-controller-manager.yaml: error converting YAML to JSON: yaml: line 16: did not find expected key
|
This sounds that we need to implement a CI action for testing the Helm installation in the e2e tests 🤔 |
I'm working on addressing the issues you've found. As to the e2e helm test, that feels out of scope for this PR. HOWEVER, we should create an issue for it. |
6329db4
to
463eec5
Compare
I think I fixed the issue where empty dictionaries Thinking about how to implement the runtime pod issue. |
408dcd1
to
a30abc9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #681 +/- ##
==========================================
+ Coverage 86.51% 86.63% +0.11%
==========================================
Files 19 19
Lines 1587 1601 +14
==========================================
+ Hits 1373 1387 +14
Misses 173 173
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. |
21bdbc5
to
55c7c83
Compare
@toddbaert I believe I've resolved all the issues you pointed out. I've largely followed your blueprint for adding labels and annotations for image pull secrets, but this also means I may have missed other places they should go. Still, I think this is worth merging to get this formally in as a supported feature. |
Amazing work. I'm reviewing and testing this now. |
Hey @cpitstick-latai ! This all looks good to me, but I tried to install via helm and I get an error (whether or not I pass any values for annotations/labels). Things I did:
Then I got: Error: UPGRADE FAILED: parse error at (open-feature-operator/templates/apps_v1_deployment_open-feature-operator-controller-manager.yaml:42): unexpected <range> in parenthesized pipeline It seems like something isn't working with the templating |
Ok I'll take a look and do some thorough testing on my end! |
dd59bfc
to
4fdcbfc
Compare
Currently, there is only one deployment, so avoid premature optimization by making these global (and follow the same pattern for imagePullSecrets) Requires more aggressive usage of the `strip-kustomize-helm.sh` script because we have to preserve existing annotations and labels from the config/manager directory. Also update labels and annotations for injected pods (FlagD sidecar, FlagD standalone, and FlagD proxy). Signed-off-by: Christopher Pitstick <cpitstick@lat.ai>
4fdcbfc
to
c6ccfc1
Compare
@toddbaert Alright I've updated it and done much more thorough testing. A couple notes:
|
The ugliest/most awkward parts are this line and its companion for annotations: I don't know how to write it better, but it does work and build properly. |
Maybe that nil check should be moved out so that it's always at least an empty map? |
952e628
to
6df99a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpitstick-latai I tested this thoroughly, including flagd-standalone and proxy modes. I did find one very small problem with the annotations if there's no annotations on a flagd deployment (there's a nil exception). I've fixed it with this commit; please make sure it makes sense to you and I will merge in the next 24hrs.
It makes sense. I did merge the if statements a little so that it would make sure the map wasn't nil before the merge check. In this way, it's always an empty map rather than a nil map that's applied to the cluster, and then we don't need nesting! We have a cyclomatic complexity issue now; however, it seems the additional conditionals tipped this function over the edge. |
6df99a3
to
facebb5
Compare
I've simplified my solution (it was really just nil annotations on the template): I now simply default the annotations on the template with the empty annotation map you created. That fixes the problem without more complexity: facebb5 |
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
55eea87
to
59686ce
Compare
Looks good to me, feel free to merge this at your convenience and when you get some teammates to review it! |
Currently, there is only one deployment, so avoid premature optimization by making these global (and follow the same pattern for imagePullSecrets)
Requires more aggressive usage of the
strip-kustomize-helm.sh
script because we have to preserve existing annotations and labels from the config/manager directory.Notes
I chose to use
maps.Copy
which requires an upgrade in some of the golang dependencies. This function was made standard in Golang 1.21, which is the version the operator currently uses, so I don't foresee issues here.Testing
There are 3 use-cases that matter here:
Label injection should happen only on the last two. Though I haven't tested those. I verified that this works on an EKS 1.27 cluster with a Calico CNI and verified sidecar injection doesn't break.