Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Detect changes made to git source in HelmRelease #1865

Merged
merged 1 commit into from
Apr 9, 2019

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Mar 26, 2019

Fixes #1850

Before this change, changes made to the git chart source in a HelmRelease did go unnoticed. Now, in case the URL or ref in the HelmRelease changes, the clone is removed so a new one can be made that matches the updated URL and ref. As a bonus, in case a HelmRelease is removed, the clone will now be removed too.

@hiddeco hiddeco changed the title Salt clone lookup name with mirror name Make Helm operator detect changes to Git source URL Mar 26, 2019
@hiddeco hiddeco added the helm label Mar 26, 2019
@hiddeco hiddeco requested a review from squaremo March 27, 2019 14:31
@hiddeco hiddeco marked this pull request as ready for review March 27, 2019 14:31
@squaremo
Copy link
Member

squaremo commented Apr 2, 2019

This is my understanding of the cause of the problem and the solution here:

  • if the git URL in a HelmRelease changes (to something that is not already being mirrored) ...
    • it won't be noticed in the mirror changes (L156 then L172 which checks if a HelmRelease uses a mirror that's changed)
    • when the ticker ticks (L244) the release will be considered in reconcileReleaseDef, then ...
      • it will find a clone against the release name clones[releaseName] at L307
      • that will be used to calculate the release -- but it's from the old git URL still, so nothing will change

This PR changes how clones are recorded, such that they use the git URL as well as the release name. That means that at (the equivalent of) [[L307]], the clone is not found, a new mirror is set up, and that will trigger a change at [[L172]] when it is ready. At [[L209]], the clone for the release won't be found, and it'll be exported again. From that point, things will continue as before.

One question that occurred to me was "what if the git branch (ref) in the HelmRelease changes, but not the URL?".

This takes a different path: when there's a change in the repo, the check at [[L172]] will succeed, and the clone will be present at [[L209]]. The check for new commits at [[L214]] may fail, if git can't construct a history between the old branch and the new branch; and this problem will likely persist. (This might be solved by just checking if the head is the same; anyway it's a side issue.) Assuming it gets past [[L214]], it'll export the repo and continue as before.

Another thing that struck me is "why not check if the git URL and ref match the clone before using it?". I.e., in reconcileReleaseDef, if the clone is found at L307, compare the HelmRelease values to the clone, and remove the clone if they don't match (then proceed with the "clone not found" branch). If it's correct, it would have the advantage of not requiring additional clean-up code.

@hiddeco hiddeco force-pushed the helm/1850-repo-change-in-helm-release branch 4 times, most recently from c485ca5 to 1e0fbc8 Compare April 4, 2019 20:41
Before this change, changes made to the git chart source in a
`HelmRelease` did go unnoticed. Now, in case the URL or ref in the
`HelmRelease` changes, the clone is removed so a new one can be made
that matches the updated URL and ref. As a bonus, in case a
`HelmRelease` is removed, the clone will now be removed too.
@hiddeco hiddeco force-pushed the helm/1850-repo-change-in-helm-release branch from 1e0fbc8 to 8171c60 Compare April 4, 2019 20:42
@hiddeco
Copy link
Member Author

hiddeco commented Apr 4, 2019

@squaremo your comment made a lot of sense and made the solution a lot simpler. PTAL whenever you feel like it.

@hiddeco hiddeco changed the title Make Helm operator detect changes to Git source URL Detect changes made to git source in HelmRelease Apr 4, 2019
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Neat! Deleting the clone for a deleted HR is a bonus 💯

@hiddeco hiddeco merged commit 634e0d6 into master Apr 9, 2019
@hiddeco hiddeco deleted the helm/1850-repo-change-in-helm-release branch April 9, 2019 10:57
@hpurmann
Copy link

Thanks to both of you!

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

Successfully merging this pull request may close these issues.

Changing repository url of HelmRelease doesn't sync
3 participants