From 006cae185678ef80c345df4920e4122356b407b4 Mon Sep 17 00:00:00 2001 From: Stavros Kois <47820033+stavros-k@users.noreply.github.com> Date: Sat, 26 Oct 2024 18:55:53 +0300 Subject: [PATCH] fix(common): set correct default for restartPolicy for cronjobs (#28356) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Description** ⚒️ Fixes #28118 **⚙️ Type of change** - [ ] ⚙️ Feature/App addition - [x] 🪛 Bugfix - [ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] 🔃 Refactor of current code **🧪 How Has This Been Tested?** **📃 Notes:** **✔️ Checklist:** - [x] ⚖️ My code follows the style guidelines of this project - [x] 👀 I have performed a self-review of my own code - [ ] #️⃣ I have commented my code, particularly in hard-to-understand areas - [x] 📄 I have made corresponding changes to the documentation - [x] ⚠️ My changes generate no new warnings - [x] 🧪 I have added tests to this description that prove my fix is effective or that my feature works - [x] ⬆️ I increased versions for any altered app according to semantic versioning - [x] I made sure the title starts with `feat(chart-name):`, `fix(chart-name):` or `chore(chart-name):` **➕ App addition** If this PR is an app addition please make sure you have done the following. - [ ] 🖼️ I have added an icon in the Chart's root directory called `icon.png` --- _Please don't blindly check all the boxes. Read them and only check those that apply. Those checkboxes are there for the reviewer to see what is this all about and the status of this PR with a quick glance._ --- .../library/common-test/ci/cron-values.yaml | 1 - .../common-test/tests/cronjob/spec_test.yaml | 5 ++++ .../tests/cronjob/validation_test.yaml | 14 ++++++++++ .../tests/ingress/homepage_test.yaml | 6 ++--- .../common-test/tests/job/spec_test.yaml | 5 ++++ .../tests/job/validation_test.yaml | 13 +++++++++ charts/library/common/Chart.yaml | 2 +- .../templates/lib/pod/_restartPolicy.tpl | 10 +++++++ charts/library/common/values.yaml | 1 - run_common_tests.sh | 27 +++++++++++++++++++ .../content/docs/common/workload/cronjob.md | 4 +++ .../src/content/docs/common/workload/job.md | 4 +++ 12 files changed, 86 insertions(+), 6 deletions(-) create mode 100755 run_common_tests.sh diff --git a/charts/library/common-test/ci/cron-values.yaml b/charts/library/common-test/ci/cron-values.yaml index b793b87f89cb..28321381e208 100644 --- a/charts/library/common-test/ci/cron-values.yaml +++ b/charts/library/common-test/ci/cron-values.yaml @@ -16,7 +16,6 @@ workload: type: CronJob schedule: "*/1 * * * *" podSpec: - restartPolicy: OnFailure containers: main: enabled: true diff --git a/charts/library/common-test/tests/cronjob/spec_test.yaml b/charts/library/common-test/tests/cronjob/spec_test.yaml index 388ca06804e2..ca4f6fc9d700 100644 --- a/charts/library/common-test/tests/cronjob/spec_test.yaml +++ b/charts/library/common-test/tests/cronjob/spec_test.yaml @@ -84,6 +84,11 @@ tests: parallelism: 5 ttlSecondsAfterFinished: 100 activeDeadlineSeconds: 100 + - documentIndex: *cronJobDoc + isSubset: + path: spec.jobTemplate.spec.template.spec + content: + restartPolicy: OnFailure - it: should set suspend to true set: diff --git a/charts/library/common-test/tests/cronjob/validation_test.yaml b/charts/library/common-test/tests/cronjob/validation_test.yaml index bb56c77add30..915cde2c11f8 100644 --- a/charts/library/common-test/tests/cronjob/validation_test.yaml +++ b/charts/library/common-test/tests/cronjob/validation_test.yaml @@ -58,3 +58,17 @@ tests: asserts: - failedTemplate: errorMessage: CronJob - Namespace [my-extra-super-duper-long-name-that-is-longer-than-63-characters] is not valid. Must start and end with an alphanumeric lowercase character. It can contain '-'. And must be at most 63 characters. + + - it: should fail with restartPolicy "Always" for CronJob + set: + workload: + workload-name: + enabled: true + primary: true + type: CronJob + schedule: "* * * * *" + podSpec: + restartPolicy: Always + asserts: + - failedTemplate: + errorMessage: Expected [restartPolicy] to be one of [Never, OnFailure] for [CronJob] but got [Always] diff --git a/charts/library/common-test/tests/ingress/homepage_test.yaml b/charts/library/common-test/tests/ingress/homepage_test.yaml index 269929af9abf..d7a587f40ba9 100644 --- a/charts/library/common-test/tests/ingress/homepage_test.yaml +++ b/charts/library/common-test/tests/ingress/homepage_test.yaml @@ -123,7 +123,7 @@ tests: gethomepage.dev/description: Helper chart to test different use cases of the common library gethomepage.dev/href: https://test-host/test-path gethomepage.dev/widget.url: https://test-release-name-common-test.test-release-namespace.svc:9443 - gethomepage.dev/icon: https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png + gethomepage.dev/icon: https://truecharts.org/img/hotlink-ok/chart-icons/common-test.webp gethomepage.dev/widget.type: commontest gethomepage.dev/pod-selector: app.kubernetes.io/instance=test-release-name,pod.lifecycle in (permanent) - documentIndex: *ingressDoc @@ -183,7 +183,7 @@ tests: gethomepage.dev/description: Helper chart to test different use cases of the common library gethomepage.dev/href: https://test-host/test-path gethomepage.dev/widget.url: http://test-release-name-common-test-my-service2.test-release-namespace.svc:80 - gethomepage.dev/icon: https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png + gethomepage.dev/icon: https://truecharts.org/img/hotlink-ok/chart-icons/common-test.webp gethomepage.dev/widget.type: commontest gethomepage.dev/pod-selector: app.kubernetes.io/instance=test-release-name,pod.lifecycle in (permanent) - documentIndex: *thirdIngressDoc @@ -205,7 +205,7 @@ tests: gethomepage.dev/name: TestReleaseName gethomepage.dev/description: Helper chart to test different use cases of the common library gethomepage.dev/href: https://test-host/test-path - gethomepage.dev/icon: https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png + gethomepage.dev/icon: https://truecharts.org/img/hotlink-ok/chart-icons/common-test.webp gethomepage.dev/pod-selector: app.kubernetes.io/instance=test-release-name,pod.lifecycle in (permanent) # Failures diff --git a/charts/library/common-test/tests/job/spec_test.yaml b/charts/library/common-test/tests/job/spec_test.yaml index b44b13b1b85e..fb6a27eb3b61 100644 --- a/charts/library/common-test/tests/job/spec_test.yaml +++ b/charts/library/common-test/tests/job/spec_test.yaml @@ -55,6 +55,11 @@ tests: parallelism: 5 ttlSecondsAfterFinished: 100 activeDeadlineSeconds: 100 + - documentIndex: *jobDoc + isSubset: + path: spec.template.spec + content: + restartPolicy: OnFailure - it: should set parallelism to 0 set: diff --git a/charts/library/common-test/tests/job/validation_test.yaml b/charts/library/common-test/tests/job/validation_test.yaml index 123f52529a92..bd6b68348149 100644 --- a/charts/library/common-test/tests/job/validation_test.yaml +++ b/charts/library/common-test/tests/job/validation_test.yaml @@ -59,3 +59,16 @@ tests: asserts: - failedTemplate: errorMessage: Job - Namespace [my-extra-super-duper-long-name-that-is-longer-than-63-characters] is not valid. Must start and end with an alphanumeric lowercase character. It can contain '-'. And must be at most 63 characters. + + - it: should fail with restartPolicy "Always" for Job + set: + workload: + workload-name: + enabled: true + primary: true + type: Job + podSpec: + restartPolicy: Always + asserts: + - failedTemplate: + errorMessage: Expected [restartPolicy] to be one of [Never, OnFailure] for [Job] but got [Always] diff --git a/charts/library/common/Chart.yaml b/charts/library/common/Chart.yaml index 090f38294608..b588b507d798 100644 --- a/charts/library/common/Chart.yaml +++ b/charts/library/common/Chart.yaml @@ -48,4 +48,4 @@ sources: - https://hub.docker.com/r/mikefarah/yq - https://hub.docker.com/r/traefik/whoami type: library -version: 25.1.0 +version: 25.1.1 diff --git a/charts/library/common/templates/lib/pod/_restartPolicy.tpl b/charts/library/common/templates/lib/pod/_restartPolicy.tpl index 1893f8d49b50..388a56080b0e 100644 --- a/charts/library/common/templates/lib/pod/_restartPolicy.tpl +++ b/charts/library/common/templates/lib/pod/_restartPolicy.tpl @@ -10,6 +10,11 @@ objectData: The object data to be used to render the Pod. {{- $policy := "Always" -}} + {{- $jobTypes := (list "Job" "CronJob") -}} + {{- if mustHas $objectData.type $jobTypes -}} + {{- $policy = "OnFailure" -}} + {{- end -}} + {{/* Initialize from the "defaults" */}} {{- with $rootCtx.Values.podOptions.restartPolicy -}} {{- $policy = tpl . $rootCtx -}} @@ -30,5 +35,10 @@ objectData: The object data to be used to render the Pod. {{- fail (printf "Expected [restartPolicy] to be [Always] for [%s] but got [%s]" $objectData.type $policy) -}} {{- end -}} + {{- if and (eq "Always" $policy) (mustHas $objectData.type $jobTypes) -}} + {{- $cronPolicies := mustWithout $policies "Always" -}} + {{- fail (printf "Expected [restartPolicy] to be one of [%s] for [%s] but got [%s]" (join ", " $cronPolicies) $objectData.type $policy) -}} + {{- end -}} + {{- $policy -}} {{- end -}} diff --git a/charts/library/common/values.yaml b/charts/library/common/values.yaml index 4e97fef18972..445259ecc384 100644 --- a/charts/library/common/values.yaml +++ b/charts/library/common/values.yaml @@ -160,7 +160,6 @@ podOptions: # If this key exists, takes precedence over the automated calculation # hostUsers: false shareProcessNamespace: false - restartPolicy: Always dnsPolicy: ClusterFirst dnsConfig: options: diff --git a/run_common_tests.sh b/run_common_tests.sh new file mode 100755 index 000000000000..20e85013a5a6 --- /dev/null +++ b/run_common_tests.sh @@ -0,0 +1,27 @@ +#!/bin/bash +# https://github.com/helm-unittest/helm-unittest + +# -- You need to install this helm plugin +# helm plugin install https://github.com/helm-unittest/helm-unittest + +common_test_path="charts/library/common-test" + +function cleanup { + if [ -d "$common_test_path/charts" ]; then + echo "🧹 Cleaning up charts..." + rm -r "$common_test_path/charts" + rm "$common_test_path/Chart.lock" + # Clean snapshots + rm -r "$common_test_path/**/__snapshot__" 2>/dev/null + fi +} + +cleanup + +echo "🔨 Building common..." +helm dependency update "$common_test_path" + +echo "🧪 Running tests..." +helm unittest --update-snapshot -f "tests/*/*.yaml" "./$common_test_path" -v ./$common_test_path/unit-values.yaml + +cleanup diff --git a/website/src/content/docs/common/workload/cronjob.md b/website/src/content/docs/common/workload/cronjob.md index dafdc9a36855..e2a555c1fff5 100644 --- a/website/src/content/docs/common/workload/cronjob.md +++ b/website/src/content/docs/common/workload/cronjob.md @@ -20,6 +20,10 @@ Replace references to `$name` with the actual name you want to use. - `.Values.workload.$name` +## Notes + +Value of `workload.$name.podSpec.restartPolicy` can **not** be `Always` for this type of workload + ## `schedule` Define the schedule diff --git a/website/src/content/docs/common/workload/job.md b/website/src/content/docs/common/workload/job.md index f3aa00b3b458..3fdaec02684d 100644 --- a/website/src/content/docs/common/workload/job.md +++ b/website/src/content/docs/common/workload/job.md @@ -20,6 +20,10 @@ Replace references to `$name` with the actual name you want to use. - `.Values.workload.$name` +## Notes + +Value of `workload.$name.podSpec.restartPolicy` can **not** be `Always` for this type of workload + --- ## `completionMode`