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.

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

ref(client): use three-way merge patch strategy #6124

Merged
merged 1 commit into from Aug 6, 2019

Conversation

@bacongobbler
Copy link
Member

commented Jul 30, 2019

closes #3805
closes #1873
rebased on top of #6085

See 489430a for the changes in this PR.

Co-Signed-by: Taylor Thomas taylor.thomas@microsoft.com
Signed-off-by: Matthew Fisher matt.fisher@microsoft.com

@hickeyma

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@bacongobbler @thomastaylor312 Will review when I get a chance.

ref(client): use three-way merge patch strategy
Co-Signed-by: Taylor Thomas <taylor.thomas@microsoft.com>
Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>

@bacongobbler bacongobbler force-pushed the bacongobbler:three-way-merge-patch branch from d75d6f2 to 18ca0dd Aug 2, 2019

return nil, types.StrategicMergePatchType, errors.Wrap(err, "unable to create patch metadata from object")
}

patch, err := strategicpatch.CreateThreeWayMergePatch(oldData, newData, currentData, patchMeta, true)

This comment has been minimized.

Copy link
@thomastaylor312

thomastaylor312 Aug 2, 2019

Collaborator

Question for anyone who comes to check this PR out: The last boolean is a flag that forces an overwrite of the incoming data. That is what we consider to be a good default behavior (the new chart always wins). However, an alternative to this would be to have the --force flag control this boolean, so the default would be if there are any conflicts, it would error out. Is there any reason to have this second option?

Personally, I think it is better to leave this how it is, but want to point it out in case there are any good use cases we haven't considered

@adamreese

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Has this been manually tested? I'll try breaking it today. The code LGTM though

@thomastaylor312

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

@adamreese We tested this fairly extensively before opening the PR, walking through various upgrade and rollback scenarios

@thomastaylor312 thomastaylor312 merged commit 4778e2b into helm:dev-v3 Aug 6, 2019

2 checks passed

DCO DCO
Details
ci/circleci: build Your tests passed on CircleCI!
Details

@bacongobbler bacongobbler deleted the bacongobbler:three-way-merge-patch branch Aug 6, 2019

@hickeyma
Copy link
Contributor

left a comment

LGTM, thanks. Tested manually with some basic changes in the cluster and then on upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.