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

pkg/helm/release/manager.go: refactor to use ThreeWayMergePatch for native k8s objects #2869

Merged
merged 4 commits into from
Apr 21, 2020

Conversation

Lucaber
Copy link
Contributor

@Lucaber Lucaber commented Apr 21, 2020

A follow-up PR to #2808 and #2777. @joelanford

In this PR we are using the Kubernetes strategic three way merge patch for patches by the helm-operator for native Kubernetes objects.
This ensures that we are using the correct merge strategy that is defined in the OpenAPI schema of the object. #2808

However we do not have the required schema information for custom resources(CRs). So we need to fallback to the previous manual json-patch.
To detect CRs we are using the same method as helm https://github.com/helm/helm/blob/b21b00978539ea8270e6b672bc1e6bc07af61475/pkg/kube/client.go#L391

@Lucaber Lucaber force-pushed the helm-patch branch 2 times, most recently from f7e8e3c to 5ebae4e Compare April 21, 2020 14:14
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great!

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2020
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Really cool collab 👍
Very nice to see the test in place.

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants