Prepare Helm release logic rewrite #477
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 6, 2022
pjbgf
reviewed
May 6, 2022
pjbgf
reviewed
May 6, 2022
pjbgf
reviewed
May 6, 2022
hiddeco
changed the title
|
Something is making the current end-to-end tests take precisely 5 minutes (and a little more) to be marked as Ready. I am unable to produce this locally however (using the same testdata), so am inclined to say it has something to do with the bash magic in CI. In any case, taking a note here, but not going to invest time in it now. As the tests are going to reworked anyway. |
darkowlzz
reviewed
May 9, 2022
darkowlzz
reviewed
May 9, 2022
| obj.Status.HelmChart = "" | ||
| } | ||
|
|
||
| if obj.DeletionTimestamp != nil { |
There was a problem hiding this comment.
This seems like a new check in all the various reconcileDelete() implementations.
Is this necessary in certain situations or just added it here for safety?
There was a problem hiding this comment.
It is new because reconcileDelete can happen both due to a change, and due to Reconcile calling it due to DeletionTimestamp != nil. Given this, it gate keeps a change from removing the finalizer.
hiddeco
force-pushed
the
prepare-rewrite
branch
2 times, most recently
from
5e6cf57 to
4a79e96
Compare
This adds an observer which wraps around a Helm storage driver, to keep track of the release metadata as written to the storage. This enables you to work with, and compare release data as persisted by Helm. Without having to rely on the result as returned by the Helm SDK. Which at times of an error, may differ from last written state. The observer does at present expect to be watching a single namespace, and was designed without working with multiple releases simultianiously into account, although this should theoretically still work. The releases are at stored in a simple map by index storage key, which are unique to the namespace. The `ObservedRelease` objects the keys hold are overwritten on sequential writes to the same release object, and returned by getter methods as deep copies. This could theoretically be changed to observing e.g. all writes, I have left this as a refinement TODO while actually implementing it in the reconciler. The same goes for the included metadata, which might be not all relevant. Signed-off-by: Hidde Beydals <[email protected]>
This moves the HelmChart template handling to a separate reconciler, with predicates detecing relevant changes. The idea is that this would both facilitate working _without_ chart templates but with references in the future, and to reduce cognitive load while working with reconciler logic. The predicate uses cmp.Equal to inspect the Chart template objects of the old and new HelmRelease object in the update vent. The reconciler uses cmp.Diff with a custom SimpleReporter, to reduce the output to a minimum. Example: ``` // Path: // -old // +new // // Path.Nested: // -removed // // Path.Other: // +added ``` The implementation on the release reconciler's end is a rough sketch, but in working shape. The foresight is that much of the reconciler will change when the release logic will be adjusted to work with the earlier introduced storage observer. Signed-off-by: Hidde Beydals <[email protected]>
This commit moves various generic bits out of the reconciler into separate modules, while adding more test coverage. Some of the logic around merging chart values from references has been improved to work with `client.Object`, instead of two separate maps. In addition, the option to override the hostname of an Artifact has been removed. It was undocumented and for testing purposes only, which these days can be better achieved by e.g. configuring the `--storage-adv-addr`. Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Plus change the tests a tiny bit to work with Gomega, and break the further API free from direct attachment to our Helm API objects. Signed-off-by: Hidde Beydals <[email protected]>
1 task
stefanprodan
reviewed
May 9, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In short (see individual commits for further detail):
dev, given the total size of the work. I am going to follow up with more PRs.