-
Notifications
You must be signed in to change notification settings - Fork 262
Conversation
KUBECTL_CHECKSUM_arm=dc60bdf00e6c7806e3c11e4f73e2ff27a603e968f22567d8c87ed5ece04263e557cb0df8d7b5196bc28a96b0b1f8ae104c503e9b9d90ce77c25e85954b54e178 | ||
KUBECTL_CHECKSUM_arm64=00a98acd51107d1cb935cfc07ca31487290412f92ac34a91ec8c7f4b802bf798a7cc9cac22978a92001641c3c99a651c7f9a9f7f64f7cc4839a9d26021667a3b | ||
|
||
KUBECTL_VERSION=v1.14.9 |
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.
Is the version bump intentional?
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.
Yes, it even has a dedicated commit (5cccc11
). The previous version did not support kubectl apply -k
.
test/e2e/10_helm_chart.bats
Outdated
# The gitconfig secret must exist and have the right value | ||
poll_until_equals "gitconfig secret" "${GITCONFIG}" "kubectl get secrets -n ${E2E_NAMESPACE} gitconfig -ojsonpath={..data.gitconfig} | base64 --decode" | ||
|
||
poll_until_true "namespace $DEMO_NAMESPACE" "kubectl describe ns/$DEMO_NAMESPACE" |
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.
Isn't this testing tiller instead of the helm operator? I would instead check with the helm client that the release is made
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.
Actually, the releases are applied below, so I would simply remove this
test/e2e/10_helm_chart.bats
Outdated
kubectl apply -f "$FIXTURES_DIR/releases" >&3 | ||
|
||
poll_until_true 'podinfo-helm-repository HelmRelease' 'kubectl -n demo get helmrelease/podinfo-helm-repository | grep -e "deployed"' | ||
poll_until_true 'podinfo-git HelmRelease' 'kubectl -n demo describe helmrelease/podinfo-git | grep -e "deployed"' |
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.
Doesn't this simply test that the releases were applied? Shouldn't we test that the operator applied them instead?
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.
No, it tests that the releases have been made by the operator and were successful, hence the | grep -e "deployed"
.
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.
Ah, sorry, I missed that. It would be cleaner to use poll_until_equals
though and extracting the status field.
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 forgot about the existence of that method while modifying what I copied from Flux. Will use that instead, much better indeed.
I think we should share the scaffolding with Flux instead of (mostly) duplicating the files. How about using a common repository and downloading an exported tarball (or even cloning the repo) locally before running the tests? It's a poor man's external library import system, but I think it's better than simply duplicating the files |
@2opremio I think for the small amount of overlap (as only |
Uhm, the basic structure of the libraries is still the same, and maybe it could be adapted to have a lower divergence (sharing more code). But I will let you decide :) |
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. Only a couple of optional change suggestions.
@2opremio agreed, but to be able to adapt it to have a lower divergence, one first needs to know what they have in common or how they utilize things, which is in my opinion something that will not be clear until both projects have a decent amount of tests. I can create a follow up issue for #103 (probably on the Flux side of issue tracking) to look into this once the operator has a decent basic test coverage. |
Mainly to support `kubectl apply -k`, but also because the current version is very old and no longer maintained.
31ce136
to
b2016a5
Compare
Scaffolding for #103.
This lacks any tooling around working with multiple Helm versions.