diff --git a/REVIEW_GUIDELINES.md b/REVIEW_GUIDELINES.md index 0419a07aaf64..b59ca0db736d 100644 --- a/REVIEW_GUIDELINES.md +++ b/REVIEW_GUIDELINES.md @@ -4,7 +4,7 @@ Anyone is welcome to review pull requests. Besides our [technical requirements]( ## Process -The process to get a pull request merged is fairly simple. First, all required tests need to pass and the contributor needs to have a signed CLA. If there is a problem with some part of the test, such as a timeout issue, please contact one of the charts repository maintainers by commenting `cc @kubernetes/charts-maintainers`. +The process to get a pull request merged is fairly simple. First, all required tests need to pass and the contributor needs to have a signed CLA. See [Charts Testing](https://github.com/kubernetes/charts/blob/master/test/README.md) for details on our CI system and how you can provide custom values for testing. If there is a problem with some part of the test, such as a timeout issue, please contact one of the charts repository maintainers by commenting `cc @kubernetes/charts-maintainers`. The charts repository uses the OWNERS files to provide merge access. If a chart has an OWNERS file, an approver listed in that file can approve the pull request. If the chart does not have an OWNERS file, an approver in the OWNERS file at the root of the repository can approve the pull request. @@ -39,7 +39,7 @@ Resources and labels should follow some conventions. The standard resource metad name: {{ template "myapp.fullname" . }} labels: app: {{ template "myapp.name" . }} - chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }} + chart: {{ template "myapp.chart" . }} release: {{ .Release.Name }} heritage: {{ .Release.Service }} ``` @@ -87,16 +87,16 @@ We officially support compatibility with the current and the previous minor vers ## Kubernetes Native Workloads. While reviewing Charts that contain workloads such as [Deployments](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/), [StatefulSets](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/), [DaemonSets](https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/) and [Jobs](https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/) the below points should be considered. These are to be seen as best practices rather than strict enforcement. - + 1. Any workload that are stateless and long running (servers) in nature are to be created as Deployments. Deployments in turn create ReplicaSets. -2. Unless there is a compelling reason, ReplicaSets or ReplicationControllers should be avoided as workload types. +2. Unless there is a compelling reason, ReplicaSets or ReplicationControllers should be avoided as workload types. 3. Workloads that are stateful in nature such as databases, key-value stores, message queues, in-memory caches are to be created as StatefulSets 4. It is recommended that Deployments and StatefulSets configure their workloads with a [Pod Disruption Budget](https://kubernetes.io/docs/concepts/workloads/pods/disruptions/) for high availability. 5. For workloads such as databases, KV stores, etc., that replicate data, it is recommended to configure interpod anti-affinity. 6. It is recommended to have a default workload update strategy configured that is suitable for this chart. -7. Batch workloads are to be created using Jobs. -8. It is best to always create workloads with the latest supported [api version](https://v1-8.docs.kubernetes.io/docs/api-reference/v1.8/) as older version are either deprecated or soon to be deprecated. -9. It is generally not advisable to provide hard [resource limits](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container) to workloads and leave it configurable unless the workload requires such quantity bare minimum to function. -10. As much as possible complex pre-app setups are configured using [init contianers](https://kubernetes.io/docs/concepts/workloads/pods/init-containers/). +7. Batch workloads are to be created using Jobs. +8. It is best to always create workloads with the latest supported [api version](https://v1-8.docs.kubernetes.io/docs/api-reference/v1.8/) as older version are either deprecated or soon to be deprecated. +9. It is generally not advisable to provide hard [resource limits](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container) to workloads and leave it configurable unless the workload requires such quantity bare minimum to function. +10. As much as possible complex pre-app setups are configured using [init contianers](https://kubernetes.io/docs/concepts/workloads/pods/init-containers/). -More [configuration](https://kubernetes.io/docs/concepts/configuration/overview/) best practices. +More [configuration](https://kubernetes.io/docs/concepts/configuration/overview/) best practices. diff --git a/test/README.md b/test/README.md index 9ecc0794f3ac..c37528c77a1b 100644 --- a/test/README.md +++ b/test/README.md @@ -27,32 +27,38 @@ The configuration of the Pull Request trigger is [in the config.json](https://gi This snippet tells Test Infra to run the [test/e2e.sh](https://github.com/kubernetes/charts/blob/master/test/e2e.sh) when testing is triggered on a pull request. The e2e.sh script will use the [Charts test image](https://github.com/kubernetes/charts/blob/master/test/Dockerfile) -to run the [test/changed.sh](https://github.com/kubernetes/charts/blob/master/test/changed.sh) script. This script +to run the [test/changed.sh](https://github.com/kubernetes/charts/blob/master/test/changed.sh) script. This script is the main logic for validation of a pull request. It intends to only test charts that have changed in this PR. The logic is as follows: -1. [Get credentials for the Kubernetes cluster used for testing.](https://github.com/kubernetes/charts/blob/master/test/changed.sh#L42) -1. [Install and initialize Helm](https://github.com/kubernetes/charts/blob/master/test/changed.sh#L47) -1. [For any charts that have changed](https://github.com/kubernetes/charts/blob/master/test/changed.sh#L62): +1. [Get credentials for the Kubernetes cluster used for testing.](https://github.com/kubernetes/charts/blob/master/test/changed.sh#L128) +1. [Install and initialize Helm](https://github.com/kubernetes/charts/blob/master/test/changed.sh#L143) +1. [For any charts that have changed](https://github.com/kubernetes/charts/blob/master/test/changed.sh#L161): - Download dependent charts, if any, with `helm dep build` - - Run `helm install` in a new namespace for this PR+build + - Run `helm install` in a new namespace for this PR build - Use the [test/verify-release.sh](https://github.com/kubernetes/charts/blob/master/test/verify-release.sh) to ensure that if any pods were launched that they get to the `Running` state - Run `helm test` on the release - Delete the release +#### Providing Custom Test Values + +Testing charts with default values may not be suitable in all cases. For instance, charts may require some values to be set which should not be part of the chart's default `values.yaml` (such as keys etc.). Furthermore, it may be desirable to test a chart with different configurations. + +In order to enable custom test values, create a directory `ci` in the chart's directory and add any number of `*-values.yaml` files to this directory. Only files with a suffix `-values.yaml` are considered. Instead of using the defaults, the chart is then installed and tested separately for each of these files using the `--values` flag. + #### Triggering -In order for the tests to be kicked off one of the -[Kubernetes member](https://github.com/orgs/kubernetes/people) must add the -"ok-to-test" label. This can also be done by commenting "/ok-to-test" on the pull request. +In order for the tests to be kicked off one of the +[Kubernetes member](https://github.com/orgs/kubernetes/people) must add the +"ok-to-test" label. This can also be done by commenting "/ok-to-test" on the pull request. This check is there to ensure that PRs are spot checked for any nefarious code. There are 2 things to check for: 1. No changes have been made to file in the test folder that unnecessarily alter the testing procedures. 1. The chart is not using images whose provenance is not traceable (usually done via the sources metadata field). -## Repo syncing +## Repo Syncing The syncing of charts to the stable and incubator repos happens from a Jenkins instance that is polling for changes to the master branch. On each change it will use the [test/repo-sync.sh](https://github.com/kubernetes/charts/blob/master/test/repo-sync.sh) diff --git a/test/changed.sh b/test/changed.sh index 3c6a4f65d6d9..3ea5cdce0761 100755 --- a/test/changed.sh +++ b/test/changed.sh @@ -17,139 +17,181 @@ set -o errexit set -o nounset set -o pipefail set -o xtrace +shopt -s nullglob git remote add k8s https://github.com/kubernetes/charts git fetch k8s master -NAMESPACE="${JOB_TYPE}-${PULL_INFO}-${BUILD_ID}" -CHANGED_FOLDERS=`git diff --find-renames --name-only $(git merge-base k8s/master HEAD) stable/ incubator/ | awk -F/ '{print $1"/"$2}' | uniq` -CURRENT_RELEASE="" +readonly NAMESPACE="${JOB_TYPE}-${PULL_INFO}-${BUILD_ID}" +readonly CHANGED_FOLDERS=$(git diff --find-renames --name-only "$(git merge-base k8s/master HEAD)" stable/ incubator/ | awk -F/ '{ print $1"/"$2 }' | uniq) # Exit early if no charts have changed -if [ -z "$CHANGED_FOLDERS" ]; then - exit 0 +if [[ -z "$CHANGED_FOLDERS" ]]; then + exit 0 fi # include the semvercompare function -curDir="$(dirname "$0")" -source "$curDir/semvercompare.sh" +readonly CURRENT_DIR="$(dirname "$0")" +source "$CURRENT_DIR/semvercompare.sh" + +release_index=1 exitCode=0 +current_release="" # Cleanup any releases and namespaces left over from the test -function cleanup { - if [ -n "$CURRENT_RELEASE" ]; then - - # Before reporting the logs from all pods provide a helm status for - # the release - helm status ${CURRENT_RELEASE} || true - - # List all logs for all containers in all pods for the namespace which was - # created for this PR test run - kubectl get pods --show-all --no-headers --namespace ${NAMESPACE} | awk '{ print $1 }' | while read line; do - if [[ $line != "" ]]; then - echo "===Details from pod $line:===" - - echo "...Description of pod $line:..." - kubectl describe pod --namespace ${NAMESPACE} ${line} || true - echo "...End of description for pod $line...\n" - - # There can be multiple containers within a pod. We need to iterate - # over each of those - containers=`kubectl get pods --show-all -o jsonpath="{.spec.containers[*].name}" --namespace ${NAMESPACE} $line` - for cname in ${containers}; do - echo "---Logs from container $cname in pod $line:---" - kubectl logs --namespace ${NAMESPACE} -c ${cname} ${line} || true - echo "---End of logs for container $cname in pod $line---\n" - done +cleanup_release() { + if [[ -n "$current_release" ]]; then + + # Before reporting the logs from all pods provide a helm status for + # the release + helm status "${current_release}" || true + + # List all logs for all containers in all pods for the namespace which was + # created for this PR test run + kubectl get pods --show-all --no-headers --namespace "$NAMESPACE" | awk '{ print $1 }' | while read -r pod; do + if [[ -n "$pod" ]]; then + printf '===Details from pod %s:===\n' "$pod" + + printf '...Description of pod %s:...\n' "$pod" + kubectl describe pod --namespace "$NAMESPACE" "$pod" || true + printf '...End of description for pod %s...\n\n' "$pod" + + # There can be multiple containers within a pod. We need to iterate + # over each of those + containers=$(kubectl get pods --show-all -o jsonpath="{.spec.containers[*].name}" --namespace "$NAMESPACE" "$pod") + for container in $containers; do + printf -- '---Logs from container %s in pod %s:---\n' "$container" "$pod" + kubectl logs --namespace "$NAMESPACE" -c "$container" "$pod" || true + printf -- '---End of logs for container %s in pod %s---\n\n' "$container" "$pod" + done + + printf '===End of details for pod %s===\n' "$pod" + fi + done + + helm delete --purge "$current_release" > cleanup_log 2>&1 || true + fi +} - echo "===End of details for pod $line===\n" - fi - done +# Cleanup any namespace left over from the test +cleanup_namespace() { + kubectl delete ns "$NAMESPACE" >> cleanup_log 2>&1 || true +} - helm delete --purge ${CURRENT_RELEASE} > cleanup_log 2>&1 || true +trap 'cleanup_release; cleanup_namespace' EXIT + +dosemvercompare() { + # Note, the trap and automatic exiting are disabled for the semver comparison + # because it catches its own errors. If the comparison fails exitCode is set + # to 1. So, trapping and exiting is re-enabled and then the exit is handled + trap - EXIT + set +e + semvercompare "$1" + trap 'cleanup_release; cleanup_namespace' EXIT + set -e + if [[ "$exitCode" == 1 ]]; then + exit 1 fi - kubectl delete ns ${NAMESPACE} >> cleanup_log 2>&1 || true } -trap cleanup EXIT - -function dosemvercompare { - # Note, the trap and automatic exiting are disabled for the semver comparison - # because it catches its own errors. If the comparison fails exitCode is set - # to 1. So, trapping and exiting is re-enabled and then the exit is handled - trap - EXIT - set +e - semvercompare ${1} - trap cleanup EXIT - set -e - if [ $exitCode == 1 ]; then - exit 1 - fi + +test_release() { + values_file="${1:-}" + current_release="$release_name-$release_index" + release_index=$((release_index + 1)) + + if [[ -n "$values_file" ]]; then + echo "Testing chart with values file: $values_file..." + helm install --timeout 600 --name "$current_release" --namespace "$NAMESPACE" --values "$values_file" . | tee install_output + else + echo "Chart does not provide test values. Testing chart with defaults..." + helm install --timeout 600 --name "$current_release" --namespace "$NAMESPACE" . | tee install_output + fi + + "$CURRENT_DIR/verify-release.sh" "$NAMESPACE" + + kubectl get pods --namespace "$NAMESPACE" + kubectl get svc --namespace "$NAMESPACE" + kubectl get deployments --namespace "$NAMESPACE" + kubectl get endpoints --namespace "$NAMESPACE" + + helm test "$current_release" + + if [[ -n "${VERIFICATION_PAUSE:-}" ]]; then + cat install_output + sleep "$VERIFICATION_PAUSE" + fi + + cleanup_release + + # Setting the current release to none to avoid the cleanup and error + # handling for a release that no longer exists. + current_release="" } if [ ! -f "${KUBECONFIG:=}" ];then - # Get credentials for test cluster - gcloud auth activate-service-account --key-file="${GOOGLE_APPLICATION_CREDENTIALS}" - gcloud container clusters get-credentials jenkins --project kubernetes-charts-ci --zone us-west1-a + # Get credentials for test cluster + gcloud auth activate-service-account --key-file="$GOOGLE_APPLICATION_CREDENTIALS" + gcloud container clusters get-credentials jenkins --project kubernetes-charts-ci --zone us-west1-a fi # Install and initialize helm/tiller -HELM_URL=https://storage.googleapis.com/kubernetes-helm -HELM_TARBALL=helm-v2.8.2-linux-amd64.tar.gz -INCUBATOR_REPO_URL=https://kubernetes-charts-incubator.storage.googleapis.com/ +readonly HELM_URL=https://storage.googleapis.com/kubernetes-helm +readonly HELM_TARBALL=helm-v2.8.2-linux-amd64.tar.gz +readonly INCUBATOR_REPO_URL=https://kubernetes-charts-incubator.storage.googleapis.com/ + pushd /opt - wget -q ${HELM_URL}/${HELM_TARBALL} - tar xzfv ${HELM_TARBALL} - PATH=/opt/linux-amd64/:$PATH + wget -q "$HELM_URL/$HELM_TARBALL" + tar xzfv "$HELM_TARBALL" + PATH="/opt/linux-amd64/:$PATH" popd helm init --client-only -helm repo add incubator ${INCUBATOR_REPO_URL} +helm repo add incubator "$INCUBATOR_REPO_URL" mkdir /opt/bin pushd /opt/bin - # Install tools to check chart versions - # Install YAML Command line reader - wget -q -O yaml https://github.com/mikefarah/yaml/releases/download/1.13.1/yaml_linux_amd64 - chmod +x yaml - - # Install SemVer testing tool - wget -q -O vert https://github.com/Masterminds/vert/releases/download/v0.1.0/vert-v0.1.0-linux-amd64 - chmod +x vert + # Install tools to check chart versions + # Install YAML Command line reader + wget -q -O yaml https://github.com/mikefarah/yaml/releases/download/1.13.1/yaml_linux_amd64 + chmod +x yaml + + # Install SemVer testing tool + wget -q -O vert https://github.com/Masterminds/vert/releases/download/v0.1.0/vert-v0.1.0-linux-amd64 + chmod +x vert popd -PATH=/opt/bin/:$PATH +PATH="/opt/bin/:$PATH" # Iterate over each of the changed charts -# Lint, install and delete -for directory in ${CHANGED_FOLDERS}; do - if [ "$directory" == "incubator/common" ]; then - continue - elif [ -d $directory ]; then - CHART_NAME=`echo ${directory} | cut -d '/' -f2` - - # A semver comparison is here as well as in the circleci tests. The circleci - # tests provide almost immediate feedback to chart authors. This test is also - # re-run right before the bot merges a PR so we can make sure the chart - # version is always incremented. - dosemvercompare ${directory} - RELEASE_NAME="${CHART_NAME:0:7}-${BUILD_ID}" - CURRENT_RELEASE=${RELEASE_NAME} - helm dep build ${directory} - helm install --timeout 600 --name ${RELEASE_NAME} --namespace ${NAMESPACE} ${directory} | tee install_output - ./test/verify-release.sh ${NAMESPACE} - kubectl get pods --namespace ${NAMESPACE} - kubectl get svc --namespace ${NAMESPACE} - kubectl get deployments --namespace ${NAMESPACE} - kubectl get endpoints --namespace ${NAMESPACE} - helm test ${RELEASE_NAME} - - if [ -n $VERIFICATION_PAUSE ]; then - cat install_output - sleep $VERIFICATION_PAUSE - fi - helm delete --purge ${RELEASE_NAME} +# Install, install and delete +for directory in $CHANGED_FOLDERS; do + if [[ "$directory" == "incubator/common" ]]; then + continue + elif [[ -d "$directory" ]]; then + chart_name=$(echo "$directory" | cut -d '/' -f2) - # Setting the current release to none to avoid the cleanup and error - # handling for a release that no longer exists. - CURRENT_RELEASE="" - fi + # A semver comparison is here as well as in the circleci tests. The circleci + # tests provide almost immediate feedback to chart authors. This test is also + # re-run right before the bot merges a PR so we can make sure the chart + # version is always incremented. + dosemvercompare "$directory" + + helm dep build "$directory" + + release_name="${chart_name:0:7}-${BUILD_ID}" + + pushd "$directory" + has_test_values= + + for values_file in ./ci/*-values.yaml; do + test_release "$values_file" + has_test_values=true + done + + if [[ -z "$has_test_values" ]]; then + test_release + fi + popd + fi done + +cleanup_namespace