🐛 fix: put annotations in deployment's pod template #1432
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -462,11 +476,11 @@ func convertToUnstructured(obj interface{}) unstructured.Unstructured { | |||
| return unstructured.Unstructured{Object: unstructuredObj} | |||
| } | |||
|
|
|||
| func containsObject(obj unstructured.Unstructured, result []client.Object) client.Object { | |||
| func findObjectByName(name string, result []client.Object) client.Object { | |||
There was a problem hiding this comment.
make sense the change to allow we mock the data like above for testDeployment 👍
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1432 +/- ##
==========================================
+ Coverage 73.45% 73.78% +0.32%
==========================================
Files 42 42
Lines 3063 3063
==========================================
+ Hits 2250 2260 +10
+ Misses 640 632 -8
+ Partials 173 171 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| By("verifying olm.targetNamespaces annotation in the deployment's pod template") | ||
| dep := findObjectByName("testDeployment", plainBundle.Objects) | ||
| Expect(dep).NotTo(BeNil()) | ||
| Expect(dep.(*appsv1.Deployment).Spec.Template.Annotations).To(HaveKeyWithValue("olm.targetNamespaces", strings.Join(watchNamespaces, ","))) |
There was a problem hiding this comment.
We are checking here the change 👍
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "testCSV", | ||
| Annotations: map[string]string{ | ||
| "olm.properties": fmt.Sprintf("[{\"type\": %s, \"value\": \"%s\"}]", property.TypeConstraint, "value"), | ||
| }, | ||
| }, | ||
| Spec: v1alpha1.ClusterServiceVersionSpec{ | ||
| InstallModes: []v1alpha1.InstallMode{{Type: v1alpha1.InstallModeTypeMultiNamespace, Supported: true}}, |
There was a problem hiding this comment.
I understand that it was removed because, after the conversion, we expect InstallStrategy, correct?
There was a problem hiding this comment.
This is the piece of the CSV that I want to configure in each of the test scenarios, so I removed it here and then each test scenario does a deep copy before setting InstallModes for that scenario.
| Namespace: installNamespace, | ||
| Name: depSpec.Name, | ||
| Labels: depSpec.Label, | ||
| Annotations: annotations, |
There was a problem hiding this comment.
@joelanford, I have a quick question: should we not keep the annotations in both places, given that we’re merging other annotations with annotations := util.MergeMaps(in.CSV.Annotations, depSpec.Spec.Template.Annotations)?
Could we not have more annotations ?
There was a problem hiding this comment.
I'm just trying to match OLMv0's behavior (which I may still not have exactly right, btw. It's complicated). We are also dealing with the etcd size limit, so I don't want to duplicate stuff more than necessary. There is a Subscription.spec.config.annotations in OLMv0 that I think propagates to the deployment's annotations. So once we have the parameterized value stuff design/implemented/supported, users will be able to provide custom annotations.
There was a problem hiding this comment.
dealing with the etcd size limit
Bit by bit, memories are resurfacing... I remember this
Make sense 👍
|
/lgtm |
Signed-off-by: Joe Lanford <[email protected]>
joelanford
force-pushed
the
fix-targetnamespace-annotation
branch
from
d7b1425 to
3064fae
Compare
Description
Reviewer Checklist