Fix normalizing fields with empty objects/slices #2576
Conversation
rquitales
changed the title
rquitales
changed the title
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov Report
@@ Coverage Diff @@
## master #2576 +/- ##
==========================================
+ Coverage 18.45% 18.73% +0.28%
==========================================
Files 47 47
Lines 9523 9532 +9
==========================================
+ Hits 1757 1786 +29
+ Misses 7667 7642 -25
- Partials 99 104 +5
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
It's a little tricky to pick up on all the context here.
- What's the purpose of pruneSlice? Why are we discarding items from the array if not in the other array?
- Does this only affect the diffing or is the pruned result used for the application of the change?
The change itself looks safe enough to me though. Looks like a good fit for a set of unit tests to help explain why we drop certain values but not others etc.
From #2445:
This is because the API server may add or remove fields to/from resources that results in semantically equivalent resources to inputs. As such, we normalize the shape of the resulting objects to prevent continuous diff'ing. |
| @@ -139,6 +139,11 @@ func getActiveClusterFromConfig(config *clientapi.Config, overrides resource.Pro | |||
| // pruneMap builds a pruned map by recursively copying elements from the source map that have a matching key in the | |||
| // target map. This is useful as a preprocessing step for live resource state before comparing it to program inputs. | |||
| func pruneMap(source, target map[string]any) map[string]any { | |||
| // If either map is nil, return nil. | |||
There was a problem hiding this comment.
Changes look good overall, but it would be good to double-check the unit tests for these functions and make sure the updated logic is adequately covered there.
There was a problem hiding this comment.
Existing unit tests passed. Have expanded on unit test coverage for these functions in a follow-up commit.
Add more unit test Fix unit tests test name/description
rquitales
force-pushed
the
rquitales/fix-empty-items
branch
from
049b680
to
8c58ca1
Compare
Proposed changes
This PR further improves the resource normalization logic.
Prior to this merge, field values that consists of an empty
map[string]interface{}or empty slice would be discarded. This means that normalizing{parentField: {childField: {} }}would result in the childField being unset as the empty object ({}) value forchildFieldis discarded. In Kubernetes resources, especially when defining NetworkPolicies and ingress/egress rules,{}!= unset field.A new test case is added to validate that normalizing NetworkPolicy resources does not result in unwanted behaviour. To do this, the GKE cluster we spin up for testing also enables the Calico network enforcement.
Related issues (optional)
Fixes: #2538