From 2e100c9b0132afd71c85bfac989121df5284de0b Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 23 Aug 2024 10:04:18 -0400 Subject: [PATCH 1/5] Check if condition and needs input for pr-builder With https://github.com/rapidsai/shared-workflows/pull/237, workflows may now specify `if: always()` to allow skipping jobs. This requires passing a `needs` input with `${{ toJSON(needs) }}`. Enforce this new rule in `rapids-check-pr-job-dependencies`, and add some tests for the tool. --- .../test_rapids-check-pr-job-dependencies.py | 209 ++++++++++++++++++ tools/rapids-check-pr-job-dependencies | 19 ++ 2 files changed, 228 insertions(+) create mode 100644 tests/test_rapids-check-pr-job-dependencies.py diff --git a/tests/test_rapids-check-pr-job-dependencies.py b/tests/test_rapids-check-pr-job-dependencies.py new file mode 100644 index 0000000..3e39530 --- /dev/null +++ b/tests/test_rapids-check-pr-job-dependencies.py @@ -0,0 +1,209 @@ +import os.path +import pytest +import subprocess +from textwrap import dedent + +TOOLS_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), "..", "tools") + + +@pytest.mark.parametrize( + ["contents", "ignored_jobs", "output", "exit_code"], + [ + ( + dedent( + """\ + jobs: + pr-builder: + needs: + - job1 + - job2 + job1: + job2: + """ + ), + None, + "pr-builder depends on all other jobs.\n", + 0, + ), + ( + dedent( + """\ + jobs: + pr-builder: + needs: + - job1 + - job2 + job1: + job2: + """ + ), + [], + "pr-builder depends on all other jobs.\n", + 0, + ), + ( + dedent( + """\ + jobs: + pr-builder: + needs: + - job1 + - job2 + job1: + job2: + job3: + """ + ), + ["job3"], + "pr-builder depends on all other jobs.\n", + 0, + ), + ( + dedent( + """\ + jobs: + pr-builder: + needs: + - job1 + - job2 + job1: + job2: + job3: + """ + ), + None, + dedent( + """\ + 'pr-builder' job is missing the following dependent jobs: + - job3 + + Update '{filename}' to include these missing jobs for 'pr-builder'. + Alternatively, you may ignore these jobs by passing them as an argument to this script. + """ + ), + 1, + ), + ( + dedent( + """\ + jobs: + pr-builder: + needs: + - job1 + - job2 + if: true + job1: + job2: + """ + ), + None, + dedent( + """\ + If 'pr-builder' job has an 'if' condition, it must be set to 'always()'. + + Update '{filename}' to set the correct 'if' condition. + """ + ), + 1, + ), + ( + dedent( + """\ + jobs: + pr-builder: + needs: + - job1 + - job2 + if: always() + job1: + job2: + """ + ), + None, + dedent( + """\ + If 'pr-builder' job has an 'if' condition, it must also set the 'needs' input to '${{{{ toJSON(needs) }}}}'. + + Update '{filename}' to add the following: + + with: + needs: ${{{{ toJSON(needs) }}}} + """ + ), + 1, + ), + ( + dedent( + """\ + jobs: + pr-builder: + needs: + - job1 + - job2 + if: always() + with: + needs: invalid + job1: + job2: + """ + ), + None, + dedent( + """\ + If 'pr-builder' job has an 'if' condition, it must also set the 'needs' input to '${{{{ toJSON(needs) }}}}'. + + Update '{filename}' to add the following: + + with: + needs: ${{{{ toJSON(needs) }}}} + """ + ), + 1, + ), + ( + dedent( + """\ + jobs: + pr-builder: + needs: + - job1 + - job2 + if: always() + with: + needs: ${{ toJSON(needs) }} + job1: + job2: + """ + ), + None, + "pr-builder depends on all other jobs.\n", + 0, + ), + ], +) +def test_rapids_check_pr_job_dependency( + tmp_path: os.PathLike, + contents: str, + ignored_jobs: list[str], + output: str, + exit_code: int, +): + filename = os.path.join(tmp_path, "pr.yaml") + with open(filename, "w") as f: + f.write(contents) + result = subprocess.run( + [ + os.path.join(TOOLS_DIR, "rapids-check-pr-job-dependencies"), + *([] if ignored_jobs is None else [" ".join(ignored_jobs)]), + + ], + env={ + **os.environ, + "WORKFLOW_FILE": filename, + }, + text=True, + capture_output=True, + ) + assert result.stdout == output.format(filename=filename) + assert result.stderr == "" + assert result.returncode == exit_code diff --git a/tools/rapids-check-pr-job-dependencies b/tools/rapids-check-pr-job-dependencies index a0e1a11..bbc0bf3 100755 --- a/tools/rapids-check-pr-job-dependencies +++ b/tools/rapids-check-pr-job-dependencies @@ -37,4 +37,23 @@ if yq -en 'env(MISSING_JOBS) | length != 0' >/dev/null 2>&1; then exit 1 fi +if if_condition="$(yq -e '.jobs.[env(PR_BUILDER_JOB_NAME)].if' "${WORKFLOW_FILE}" 2>/dev/null)"; then + if [[ "${if_condition}" != "always()" ]]; then + echo "If '${PR_BUILDER_JOB_NAME}' job has an 'if' condition, it must be set to 'always()'." + echo "" + echo "Update '${WORKFLOW_FILE}' to set the correct 'if' condition." + exit 1 + fi + + if yq -e '.jobs.[env(PR_BUILDER_JOB_NAME)].with.needs != "${{ toJSON(needs) }}"' "${WORKFLOW_FILE}" >/dev/null 2>&1; then + echo "If '${PR_BUILDER_JOB_NAME}' job has an 'if' condition, it must also set the 'needs' input to '\${{ toJSON(needs) }}'." + echo "" + echo "Update '${WORKFLOW_FILE}' to add the following:" + echo "" + echo "with:" + echo " needs: \${{ toJSON(needs) }}" + exit 1 + fi +fi + echo "${PR_BUILDER_JOB_NAME} depends on all other jobs." From 5c95bdeb8d31e2117336a411e83b824de74e0288 Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 23 Aug 2024 10:08:45 -0400 Subject: [PATCH 2/5] Ignore warning about expression expansion --- tools/rapids-check-pr-job-dependencies | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/rapids-check-pr-job-dependencies b/tools/rapids-check-pr-job-dependencies index bbc0bf3..a420dc7 100755 --- a/tools/rapids-check-pr-job-dependencies +++ b/tools/rapids-check-pr-job-dependencies @@ -45,6 +45,7 @@ if if_condition="$(yq -e '.jobs.[env(PR_BUILDER_JOB_NAME)].if' "${WORKFLOW_FILE} exit 1 fi + # shellcheck disable=SC2016 if yq -e '.jobs.[env(PR_BUILDER_JOB_NAME)].with.needs != "${{ toJSON(needs) }}"' "${WORKFLOW_FILE}" >/dev/null 2>&1; then echo "If '${PR_BUILDER_JOB_NAME}' job has an 'if' condition, it must also set the 'needs' input to '\${{ toJSON(needs) }}'." echo "" From ac4855a24d467639a0183fded544f7080700094c Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 23 Aug 2024 10:12:47 -0400 Subject: [PATCH 3/5] Rename function --- tests/test_rapids-check-pr-job-dependencies.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_rapids-check-pr-job-dependencies.py b/tests/test_rapids-check-pr-job-dependencies.py index 3e39530..1ec9a88 100644 --- a/tests/test_rapids-check-pr-job-dependencies.py +++ b/tests/test_rapids-check-pr-job-dependencies.py @@ -181,7 +181,7 @@ ), ], ) -def test_rapids_check_pr_job_dependency( +def test_rapids_check_pr_job_dependencies( tmp_path: os.PathLike, contents: str, ignored_jobs: list[str], From 3622105d6874e4c95d1156fcfe800c49afb993ab Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 23 Aug 2024 10:13:08 -0400 Subject: [PATCH 4/5] Remove stray newline --- tests/test_rapids-check-pr-job-dependencies.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_rapids-check-pr-job-dependencies.py b/tests/test_rapids-check-pr-job-dependencies.py index 1ec9a88..1730a32 100644 --- a/tests/test_rapids-check-pr-job-dependencies.py +++ b/tests/test_rapids-check-pr-job-dependencies.py @@ -195,7 +195,6 @@ def test_rapids_check_pr_job_dependencies( [ os.path.join(TOOLS_DIR, "rapids-check-pr-job-dependencies"), *([] if ignored_jobs is None else [" ".join(ignored_jobs)]), - ], env={ **os.environ, From 61dc4377418da78f986259e0a28bc7b261f75fff Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 23 Aug 2024 10:20:32 -0400 Subject: [PATCH 5/5] Add another test --- .../test_rapids-check-pr-job-dependencies.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/test_rapids-check-pr-job-dependencies.py b/tests/test_rapids-check-pr-job-dependencies.py index 1730a32..f9448c0 100644 --- a/tests/test_rapids-check-pr-job-dependencies.py +++ b/tests/test_rapids-check-pr-job-dependencies.py @@ -132,6 +132,33 @@ ), 1, ), + ( + dedent( + """\ + jobs: + pr-builder: + needs: + - job1 + - job2 + if: always() + with: + job1: + job2: + """ + ), + None, + dedent( + """\ + If 'pr-builder' job has an 'if' condition, it must also set the 'needs' input to '${{{{ toJSON(needs) }}}}'. + + Update '{filename}' to add the following: + + with: + needs: ${{{{ toJSON(needs) }}}} + """ + ), + 1, + ), ( dedent( """\