-
Notifications
You must be signed in to change notification settings - Fork 669
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
Add helm test #527
Add helm test #527
Conversation
Hi @pravarag. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
test/run-helm-tests.sh
Outdated
@@ -0,0 +1,44 @@ | |||
# Copyright 2017 The Kubernetes Authors. |
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.
# Copyright 2017 The Kubernetes Authors. | |
# Copyright 2021 The Kubernetes Authors. |
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.
Updated the license year
/kind cleanup |
test/run-helm-tests.sh
Outdated
export KUBECONFIG="$(kind get kubecofnig-path)" | ||
docker pull kubernetes/pause | ||
kind load docker-image kubernetes/pause | ||
kind get kubeconfig > /tmp/admin.conf | ||
export KUBECONFIG="/tmp/admin.conf" | ||
mkdir -p ~/gopath/src/sigs.k8s.io/ | ||
mv ~/gopath/src/github.com/kubernetes-sigs/descheduler ~/gopath/src/sigs.k8s.io/. |
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.
export KUBECONFIG="$(kind get kubecofnig-path)" | |
docker pull kubernetes/pause | |
kind load docker-image kubernetes/pause | |
kind get kubeconfig > /tmp/admin.conf | |
export KUBECONFIG="/tmp/admin.conf" | |
mkdir -p ~/gopath/src/sigs.k8s.io/ | |
mv ~/gopath/src/github.com/kubernetes-sigs/descheduler ~/gopath/src/sigs.k8s.io/. |
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.
@seanmalloy shall I keep this line: export KUBECONFIG="$(kind get kubecofnig-path)"
or remove that one too? I'm guessing since we aren't running kubectl
commands so that might not be required right?
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.
Updated the code by removing mentioned lines.
test/run-helm-tests.sh
Outdated
mkdir -p ~/gopath/src/sigs.k8s.io/ | ||
mv ~/gopath/src/github.com/kubernetes-sigs/descheduler ~/gopath/src/sigs.k8s.io/. | ||
fi | ||
PRJ_PREFIX="sigs.k8s.io/descheduler" |
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.
PRJ_PREFIX="sigs.k8s.io/descheduler" |
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.
Done
test/run-helm-tests.sh
Outdated
#!/bin/bash | ||
|
||
# This runs helm tests for a release using `helm test` command | ||
if [[ -n "${KIND_E2E}" ]]; then |
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 don't think we need this conditional.
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.
Removed the conditional with latest push.
@seanmalloy I've updated the code as per review comments. Also, I'm guessing the script should run during any changes to PR are being done, so is that the only way to test it or I can still test it in my local? |
Let's start with testing it locally. |
test/run-helm-tests.sh
Outdated
curl -Lo kubectl https://storage.googleapis.com/kubernetes-release/release/${K8S_VERSION}/bin/linux/amd64/kubectl && chmod +x kubectl && sudo mv kubectl /usr/local/bin/ | ||
wget https://github.com/kubernetes-sigs/kind/releases/download/v0.9.0/kind-linux-amd64 |
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 don't think we need kubectl
, so I believe these lines can be removed.
curl -Lo kubectl https://storage.googleapis.com/kubernetes-release/release/${K8S_VERSION}/bin/linux/amd64/kubectl && chmod +x kubectl && sudo mv kubectl /usr/local/bin/ | |
wget https://github.com/kubernetes-sigs/kind/releases/download/v0.9.0/kind-linux-amd64 |
If you come up with a reason why running helm tests would need kubectl
then these lines do not need to be removed.
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've removed kubectl download as suggested above.
test/run-helm-tests.sh
Outdated
#!/bin/bash | ||
|
||
# This runs helm tests for a release using `helm test` command | ||
K8S_VERSION=${KUBERNETES_VERSION:-v1.18.2} |
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.
K8S_VERSION=${KUBERNETES_VERSION:-v1.18.2} | |
K8S_VERSION=${KUBERNETES_VERSION:-v1.20.2} |
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.
Updated the K8s version with latest push.
@seanmalloy I've tested the changes for both the scenarios (helm test failed & helm test passed) and below are the logs, For failure scenario:
So above, the helm-test failed as the command requires Success scenario,
In above logs, we can ignore the error for |
@pravarag the changes you have so far looks good. I think you just need to create a helm test file now. See below link for an example. You could create a file named |
@seanmalloy thanks for suggesting creation of new test for verifying Also, one observation - while trying to deploy the helm chart today in one of my local kind cluster I noticed the pod created by cronJob is not getting created and getting
I'm thinking if it could be a problem specific to my cluster or in general? |
@seanmalloy so I deployed the helm chart for v1.20 release as suggested here link and I can see that the |
yes, I believe that is a good first test. |
I suggest modifying the script with the below commands
This will do a container image build and load it into the kind cluster. This will ensure that changes in the Dockerfile and helm chart are working together which will avoid the above mentioned |
dbf5ae1
to
4bc99c3
Compare
I've updated code as per review comments @seanmalloy |
test/run-helm-tests.sh
Outdated
export PATH=$PATH:$PWD | ||
kind create cluster --image kindest/node:"${K8S_VERSION}" --config=./hack/kind_config.yaml | ||
kind load docker-image descheduler:helm-test | ||
helm install descheduler-ci --set image.repository=descheduler,image.tag=helm-test --namespace kube-system ./charts/descheduler |
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.
Last change for this shell script ...
We want to be able to test different versions of the descheduler helm chart. For example a released version of the chart with released version of the container image. The CLI options you have are correct, and we want these to be the defaults.
So --set image.repository=descheduler
needs a variable that can be overridden, --set image.tag=helm-test
needs a variable. and the last parameters ./charts/descheduler
needs a variable.
Let me know if you need more details. Thanks!
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.
Updated with the latest push.
curl -LO https://storage.googleapis.com/kubernetes-release/release/$(curl -s https://storage.googleapis.com/kubernetes-release/release/stable.txt)/bin/linux/amd64/kubectl && | ||
chmod +x ./kubectl && | ||
mv ./kubectl /usr/local/bin/kubectl && | ||
/usr/local/bin/kubectl get pods --namespace kube-system --token "$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" | grep "Completed" |
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.
/usr/local/bin/kubectl get pods --namespace kube-system --token "$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" | grep "Completed" | |
/usr/local/bin/kubectl get pods --namespace kube-system --token "$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" | grep "descheduler" | grep "Completed" |
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.
Updated as suggested above.
/ok-to-test |
@pravarag one more thing. Please add some documentation to https://github.com/kubernetes-sigs/descheduler/blob/master/docs/contributor-guide.md with instructions for running the helm tests. Thanks! |
6a8e740
to
c05d36d
Compare
I've updated the code as per review comments and have added to the documentation as well @seanmalloy |
docs/contributor-guide.md
Outdated
@@ -39,5 +39,18 @@ make test-unit | |||
make test-e2e | |||
``` | |||
|
|||
## Run Helm tests |
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.
## Run Helm tests | |
## Run Helm Tests |
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.
Updated with latest push
docs/contributor-guide.md
Outdated
@@ -39,5 +39,18 @@ make test-unit | |||
make test-e2e | |||
``` | |||
|
|||
## Run Helm tests | |||
We can run the helm test for a particular descheduler release by setting below variables, |
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.
We can run the helm test for a particular descheduler release by setting below variables, | |
Run the helm test for a particular descheduler release by setting below variables, |
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.
Updated with latest push.
docs/contributor-guide.md
Outdated
## Run Helm tests | ||
We can run the helm test for a particular descheduler release by setting below variables, | ||
``` | ||
HELM_IMGAE_REPO="descheduler" |
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.
HELM_IMGAE_REPO="descheduler" | |
HELM_IMAGE_REPO="descheduler" |
docs/contributor-guide.md
Outdated
``` | ||
HELM_IMGAE_REPO="descheduler" | ||
HELM_IMAGE_TAG="helm-test" | ||
CHARTS_PATH="./charts/descheduler" |
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.
CHARTS_PATH="./charts/descheduler" | |
HELM_CHART_LOCATION="./charts/descheduler" |
test/run-helm-tests.sh
Outdated
HELM_IMAGE_REPO="descheduler" | ||
HELM_IMAGE_TAG="helm-test" | ||
CHARTS_PATH="./charts/descheduler" | ||
VERSION=helm-test make image |
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.
HELM_IMAGE_REPO="descheduler" | |
HELM_IMAGE_TAG="helm-test" | |
CHARTS_PATH="./charts/descheduler" | |
VERSION=helm-test make image | |
IMAGE_REPO=${HELM_IMAGE_REPO:-descheduler} | |
IMAGE_TAG=${HELM_IMAGE_TAG:-helm-test} | |
CHART_LOCATION=${HELM_CHART_LOCATION:-./charts/descheduler} | |
VERSION=helm-test make image |
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.
Updated.
test/run-helm-tests.sh
Outdated
export PATH=$PATH:$PWD | ||
kind create cluster --image kindest/node:"${K8S_VERSION}" --config=./hack/kind_config.yaml | ||
kind load docker-image descheduler:helm-test | ||
helm install descheduler-ci --set image.repository="${HELM_IMAGE_REPO}",image.tag="${HELM_IMAGE_TAG}" --namespace kube-system "${CHARTS_PATH}" |
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.
helm install descheduler-ci --set image.repository="${HELM_IMAGE_REPO}",image.tag="${HELM_IMAGE_TAG}" --namespace kube-system "${CHARTS_PATH}" | |
helm install descheduler-ci --set image.repository="${IMAGE_REPO}",image.tag="$IMAGE_TAG}" --namespace kube-system "${CHART_LOCATION}" |
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.
Code updated with latest push.
@pravarag I added some additional inline suggestions. I believe these are the last set of changes required before this PR gets merged. I'm hoping my suggestions result in a working script. :-) Also, thanks again for your help. You are doing a great job. |
@seanmalloy thanks for your review and suggestions, I've updated the code as per review comments. Also one more thing, do we need to add a separate CI check like you have mentioned here: #440 (comment) ? |
We can sort out adding the helm test CI check after this PR merges. Being able to run the helm tests locally is the first step which is what this PR does. One thing to note is that I'm having some problems starting up Nice work! /lgtm |
@seanmalloy thanks for the review, let me know if any more testing is required from my end for the same. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pravarag, seanmalloy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR Fixes: #440
As part of this PR, we are enabling support for
helm test
which will be run as part of CI. A new script to run helm tests has been added undertests/
directory using the kind cluster. Also, Makefile has been updated to add support forhelm test
as a separate check.