Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Remove empty keys when merging unstructured resources #2332

Merged
merged 2 commits into from
Mar 16, 2023
Merged

fix: Remove empty keys when merging unstructured resources #2332

merged 2 commits into from
Mar 16, 2023

Conversation

rquitales
Copy link
Member

When merging client-side and server-side patches for diffing, we need to remove any empty/removed keys from the server-side patch in the client-side patch as well.

Without this fix, any external changes (eg. kubectl or controllers) to resources that removes fields would not be re-set on the next pulumi up.

Partially fixes: #2331 as we still need an upstream fix (darccio/mergo#231) to be merged.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@rquitales rquitales force-pushed the fix-merge-empty-maps branch from 79fe424 to f0e1e97 Compare March 8, 2023 19:54
@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

lblackstone
lblackstone requested changes Mar 8, 2023
View reviewed changes
Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me, but will need the mergo dependency to be updated before merging the PR.

Did you confirm that this change fixes the bug in #2331? It would be great if we could cover this case in a test. Perhaps https://github.com/pulumi/pulumi-kubernetes/tree/master/tests/sdk/nodejs/server-side-apply could be extended to do that?

@rquitales rquitales force-pushed the fix-merge-empty-maps branch 2 times, most recently from 9e895b8 to 1f2cfd5 Compare March 16, 2023 03:09
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

1 similar comment
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@rquitales
Copy link
Member Author

This looks right to me, but will need the mergo dependency to be updated before merging the PR.

Did you confirm that this change fixes the bug in #2331? It would be great if we could cover this case in a test. Perhaps https://github.com/pulumi/pulumi-kubernetes/tree/master/tests/sdk/nodejs/server-side-apply could be extended to do that?

Yes, I have confirmed that this change fixes the underlying issue. Have updated mergo now that the upstream PR is merged.

Also added a new regression test for this instead of expanding on the existing server-side-apply test case since we need to run external commands in-between pulumi program runs.

@rquitales rquitales force-pushed the fix-merge-empty-maps branch from 1f2cfd5 to ba2030e Compare March 16, 2023 03:23
rquitales
rquitales commented Mar 16, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

lblackstone
lblackstone approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 🎉

kpitzen
kpitzen approved these changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@kpitzen kpitzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the Java worktree issue is dependent upon #2337 being merged first, and then this rebased on that. Pending that being done, LGTM!

@rquitales
fix: Remove empty keys when merging unstructured resources
78c61ae 
When merging client-side and server-side patches for diffing, we need to remove
any empty/removed keys from the server-side patch in the client-side patch as well.

Fixes: #2331
@rquitales rquitales force-pushed the fix-merge-empty-maps branch from ba2030e to dc76730 Compare March 16, 2023 17:10
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@rquitales rquitales enabled auto-merge (squash) March 16, 2023 17:46
@rquitales rquitales force-pushed the fix-merge-empty-maps branch from dc76730 to 779afb9 Compare March 16, 2023 17:56
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@rquitales rquitales merged commit cdf883b into master Mar 16, 2023
@rquitales rquitales deleted the fix-merge-empty-maps branch March 16, 2023 18:41
@squaremo squaremo mentioned this pull request Mar 17, 2023
Closed
@KenjiYabuki KenjiYabuki mentioned this pull request Jul 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lblackstone lblackstone lblackstone approved these changes

@kpitzen kpitzen kpitzen approved these changes

@thomas11 thomas11 thomas11 approved these changes

@squaremo squaremo Awaiting requested review from squaremo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

External changes to labels and annotations aren't reverted with server side apply.
4 participants
@rquitales @thomas11 @kpitzen @lblackstone