fix: Server side diff now works correctly with some fields removal #640
Conversation
andrii-korotkov-verkada
force-pushed
the
20792-fix-server-side-diff-with-fields-removal
branch
from
0f7c3cb to
f761215
Compare
andrii-korotkov-verkada
requested review from
leoluz,
pasha-codefresh,
crenshaw-dev,
agaudreault and
ishitasequeira
andrii-korotkov-verkada
force-pushed
the
20792-fix-server-side-diff-with-fields-removal
branch
from
f761215 to
f171197
Compare
I'll do the testing. |
andrii-korotkov-verkada
force-pushed
the
20792-fix-server-side-diff-with-fields-removal
branch
from
f171197 to
d6cc3eb
Compare
|
ArgoCD PR argoproj/argo-cd#20842, tests passed. |
andrii-korotkov-verkada
force-pushed
the
20792-fix-server-side-diff-with-fields-removal
branch
from
d6cc3eb to
d4b29ea
Compare
|
@crenshaw-dev, I've addressed your comments. Code Analysis complains that configs are not perfect, but that's okay since they aren't deployed anywhere. |
|
@andrii-korotkov-verkada wanna try adding this to a new sonar-project.properties file in the root of the repo? # Exclude test manifests from analysis
sonar.kubernetes.exclusions=pkg/diff/testdata/** |
|
Per discussion with @leoluz, I'd spend more time to understand why the Structured Merge Diff library's Merge doesn't derive key fields from paths, since the info is available. I may submit a ticket to their community with a request to update the library. |
|
I've filed kubernetes-sigs/structured-merge-diff#273 |
andrii-korotkov-verkada
force-pushed
the
20792-fix-server-side-diff-with-fields-removal
branch
from
d4b29ea to
e881697
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Structured-merge-diff PR with the fix is now merged: |
Helps with argoproj/argo-cd#20792 Removed and modified sets may only contain the fields that changed, not including key fields like "name". This can cause merge to fail, since it expects those fields to be present if they are present in the predicted live. Fortunately, we can inspect the set and derive the key fields necessary. Then they can be added to the set and used during a merge. Also, have a new test which fails before the fix, but passes now. Failure of the new test before the fix ``` Error: Received unexpected error: error removing non config mutations for resource Deployment/nginx-deployment: error reverting webhook removed fields in predicted live resource: .spec.template.spec.containers: element 0: associative list with keys has an element that omits key field "name" (and doesn't have default value) Test: TestServerSideDiff/will_test_removing_some_field_with_undoing_changes_done_by_webhook ``` Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada
force-pushed
the
20792-fix-server-side-diff-with-fields-removal
branch
from
4add644 to
91c72b1
Compare
sonar-project.properties
Outdated
| @@ -0,0 +1,2 @@ | |||
| # Exclude test manifests from analysis | |||
| sonar.kubernetes.exclusions=pkg/diff/testdata/** | |||
There was a problem hiding this comment.
This doesn't seem to be working
Signed-off-by: Andrii Korotkov <[email protected]>
|
leoluz
dismissed
crenshaw-dev’s stale review
Confirmed with Michael over Slack
Helps with argoproj/argo-cd#20792
Removed and modified sets may only contain the fields that changed, not including key fields like "name". This can cause merge to fail, since it expects those fields to be present if they are present in the predicted live. Fortunately, structured merge diff folks have added an option to extracting items to add key fields, and we are going to use that.
Failure of the new test before the fix