-
Notifications
You must be signed in to change notification settings - Fork 265
release: allow MaxHistory to be overriden for a HelmRelease #235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add the CRD change in the Helm chart to make sure we do not forget about this later on?
d99074a
to
bd7548c
Compare
@hiddeco added to CRD I assume this was manual as I had done? |
bf9490d
to
79b9cef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splendid! 🥇
79b9cef
to
2d48456
Compare
test/e2e/25_max_history.bats
Outdated
count=$(k get secret -n $DEMO_NAMESPACE --output name | grep 'sh.helm.release.v1.podinfo-helm-repository' | wc -l) | ||
[ "$count" -le 10 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is wiser to not use our human knowledge here about the configured storage mechanism but instead exec
into the running container and use the helm3
binary there to get this information. This would make the end-to-end tests still succeed if for some reason the storage mechanism is configured differently.
I also think the stricter [ "$count" -eq 10 ]
can be used, given we use poll_until_equals
in the for
loop and there should thus be no divergence due to e.g. the operator not being ready with processing.
2d48456
to
f53a441
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for adding the end-to-end tests. ⭐
Currently the --history-max=10 was hard-coded and not user configurable, this adds
maxHistory
to the HelmRelease CRD to allow a user to override this if desired.@hiddeco hoping I got all the required bits this is my first CRD related PR let me know what I am missing.
Closes #224