🐛 OCPBUGS-76381, OCPBUGS-76383: fix(deploymentConfig): honor empty affinity objects as erasure requests like OLMv0#2591
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR updates the registryv1 bundle deployment rendering logic to match OLMv0’s “erasure” semantics for deploymentConfig.affinity, ensuring that explicitly empty affinity objects clear previously-applied affinity rather than being treated as a no-op.
Changes:
- Update
applyAffinityConfigto treat emptyAffinity/ empty affinity sub-types (e.g.,NodeAffinity: {}) as erase operations. - Add/extend unit and e2e coverage for “empty affinity erases” behavior.
- Add new regression manifest fixtures for empty-affinity rendering cases.
Reviewed changes
Copilot reviewed 26 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/operator-controller/rukpak/render/registryv1/generators/generators.go | Implement OLMv0-like affinity erasure semantics for empty affinity objects/sub-objects. |
| internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go | Add unit tests covering empty affinity and empty sub-type erasure behavior. |
| test/e2e/features/install.feature | Add e2e scenarios validating that empty affinity config erases previously applied affinity (and sub-type-only erasure). |
| test/regression/convert/generate-manifests.go | Add regression generation cases for empty affinity and empty affinity subtype. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/00_clusterrole_argocd-operator-metrics-reader.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/01_clusterrole_argocd-operator.v0-1dhiybrldl1gyksid1dk2dqjsc72psdybc7iyvse5gpx.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/02_clusterrole_argocd-operator.v0.-3gkm3u8zfarktdile5wekso69zs9bgzb988mhjm0y6p.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/03_clusterrolebinding_argocd-operator.v0-1dhiybrldl1gyksid1dk2dqjsc72psdybc7iyvse5gpx.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/04_clusterrolebinding_argocd-operator.v0.-3gkm3u8zfarktdile5wekso69zs9bgzb988mhjm0y6p.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/05_configmap_argocd-operator-manager-config.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/06_customresourcedefinition_applications.argoproj.io.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/08_customresourcedefinition_appprojects.argoproj.io.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/09_customresourcedefinition_argocdexports.argoproj.io.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/11_deployment_argocd-operator-controller-manager.yaml | New expected output fixture showing affinity erased in rendered Deployment. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/12_service_argocd-operator-controller-manager-metrics-service.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity/13_serviceaccount_argocd-operator-controller-manager.yaml | New expected output fixture for empty-affinity rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/00_clusterrole_argocd-operator-metrics-reader.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/01_clusterrole_argocd-operator.v0-1dhiybrldl1gyksid1dk2dqjsc72psdybc7iyvse5gpx.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/02_clusterrole_argocd-operator.v0.-3gkm3u8zfarktdile5wekso69zs9bgzb988mhjm0y6p.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/03_clusterrolebinding_argocd-operator.v0-1dhiybrldl1gyksid1dk2dqjsc72psdybc7iyvse5gpx.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/04_clusterrolebinding_argocd-operator.v0.-3gkm3u8zfarktdile5wekso69zs9bgzb988mhjm0y6p.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/05_configmap_argocd-operator-manager-config.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/08_customresourcedefinition_appprojects.argoproj.io.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/09_customresourcedefinition_argocdexports.argoproj.io.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/11_deployment_argocd-operator-controller-manager.yaml | New expected output fixture showing nodeAffinity erased while other affinity parts remain. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/12_service_argocd-operator-controller-manager-metrics-service.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
| test/regression/convert/testdata/expected-manifests/argocd-operator.v0.6.0/with-empty-affinity-subtype/13_serviceaccount_argocd-operator-controller-manager.yaml | New expected output fixture for empty-affinity-subtype rendering case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
934ab7d to
c15cd9b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 32 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @tmshort Thank you !!
That is how it is today, but for sure we can look to see if we can improve that in follow up. |
66821ae to
0400191
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 32 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0400191 to
0b37853
Compare
0b37853 to
5cb23b4
Compare
…rasure behavior - Fixed isNodeAffinityEmpty() to treat non-nil RequiredDuringSchedulingIgnoredDuringExecution with empty NodeSelectorTerms as empty (handles YAML unmarshaling edge case) - Added unit test for empty nodeSelectorTerms scenario Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5cb23b4 to
a942d88
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2591 +/- ##
==========================================
- Coverage 68.86% 68.85% -0.01%
==========================================
Files 139 139
Lines 9872 9930 +58
==========================================
+ Hits 6798 6837 +39
- Misses 2557 2578 +21
+ Partials 517 515 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/lgtm |
0a6b9de
into
operator-framework:main
Problem
OLMv1 does not match OLMv0 when users pass empty affinity objects in
deploymentConfig.affinity: {}should erase all affinity from the deployment. OLMv1 keeps the bundle affinity instead.affinity: {nodeAffinity: {}}should erase onlynodeAffinity. OLMv1 leaves an emptynodeAffinity: {}in the manifest.Why do we need to do it?
Current OLMv1 does not fully erase it, so the bundle’s affinity can remain, or an empty nodeAffinity: {} is left behind.
This ensures affinity: {} removes all affinity and empty subfields such as nodeAffinity: {} remove only that section, instead of preserving bundle affinity or leaving empty objects behind in the rendered manifest.
Solution
Update
applyAffinityConfigto follow OLMv0OverrideDeploymentAffinitybehavior:nil→ do not touch{}→ erase all affinity{nodeAffinity: {}}→ erasenodeAffinity, keep others{nodeAffinity: {...}}→ replacenodeAffinity, keep othersBefore
After