From d9183f0587950b8bdaebbc1ffe321965cb026397 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 18 Jun 2024 15:17:02 +0200 Subject: [PATCH 1/2] cirrus.yml: implement skips based on source changes We do not have to test everything for each PR, we can know based on the source if we changed (i.e. machine code) and only run the tests then. This implements it as skip conditions, due to the nature of yaml files we unfortunately cannot deduplicate everything, i.e. the is PR check and danger files apply to everything but as skip is only a single yaml string we cannot deduplicate parts of that string. If anyone knows a way to achieve this I like to hear it. For now I implemented this for int, system, bud and machine tests. Once we are more comfortable with this I plan on adding it to other tests as well. This will replace the current _bail_if_test_can_be_skipped logic as it covers more, marks tasks actually skipped in the github UI and works even for the windows/macos machine tests. Signed-off-by: Paul Holzinger --- .cirrus.yml | 39 ++++++++++++++++++++++++++++++ contrib/cirrus/cirrus_yaml_test.py | 15 ++++++++++++ 2 files changed, 54 insertions(+) diff --git a/.cirrus.yml b/.cirrus.yml index 78d9acb205..d58e74d48c 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -639,6 +639,15 @@ local_integration_test_task: &local_integration_test_task alias: local_integration_test # Docs: ./contrib/cirrus/CIModes.md only_if: *not_tag_branch_build_docs_machine + # skip when: - it is a PR (we never want to skip on nightly tests); and + # - no danger files are changed; and + # - when no int test code is changed; and + # - NOT (source code is changed AND NOT only test files) + skip: &skip_int_test >- + $CIRRUS_PR != '' && + !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && + !changesInclude('test/e2e/**', 'test/utils/**') && + !(changesInclude('**/*.go', '**/*.c') && !changesIncludeOnly('test/**', 'pkg/machine/e2e/**')) depends_on: *build matrix: *platform_axis # integration tests scale well with cpu as they are parallelized @@ -680,6 +689,7 @@ container_integration_test_task: alias: container_integration_test # Docs: ./contrib/cirrus/CIModes.md only_if: *not_tag_branch_build_docs_machine + skip: *skip_int_test depends_on: *build matrix: &fedora_vm_axis - env: @@ -708,6 +718,7 @@ rootless_integration_test_task: alias: rootless_integration_test # Docs: ./contrib/cirrus/CIModes.md only_if: *not_tag_branch_build_docs_machine + skip: *skip_int_test depends_on: *build matrix: *platform_axis gce_instance: *fastvm @@ -731,6 +742,13 @@ podman_machine_task: $CIRRUS_CHANGE_TITLE !=~ '.*CI:DOCS.*' && $CIRRUS_CHANGE_TITLE !=~ '.*CI:BUILD.*' ) || $CIRRUS_CRON == "main" + # skip when: - it is a PR (we never want to skip on nightly tests); and + # - no danger files are changed; and + # - no machine code files are changed + skip: &skip_machine_test >- + $CIRRUS_PR != '' && + !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && + !changesInclude('cmd/podman/machine/**', 'pkg/machine/**', '**/*machine*.go') depends_on: *build ec2_instance: image: "${VM_IMAGE_NAME}" @@ -752,6 +770,7 @@ podman_machine_aarch64_task: name: *std_name_fmt alias: podman_machine_aarch64 only_if: *machine_cron_not_tag_build_docs + skip: *skip_machine_test depends_on: *build ec2_instance: <<: *standard_build_ec2_aarch64 @@ -773,6 +792,7 @@ podman_machine_windows_task: # Only run for non-docs/copr PRs and non-release branch builds # and never for tags. Docs: ./contrib/cirrus/CIModes.md only_if: *machine_cron_not_tag_build_docs + skip: *skip_machine_test depends_on: *build ec2_instance: <<: *windows @@ -797,6 +817,7 @@ podman_machine_mac_task: name: *std_name_fmt alias: podman_machine_mac only_if: *machine_cron_not_tag_build_docs + skip: *skip_machine_test depends_on: *build persistent_worker: *mac_pw env: @@ -849,6 +870,15 @@ local_system_test_task: &local_system_test_task $CIRRUS_CHANGE_TITLE !=~ '.*CI:DOCS.*' && $CIRRUS_CHANGE_TITLE !=~ '.*CI:BUILD.*' && $CIRRUS_CHANGE_TITLE !=~ '.*CI:MACHINE.*' + # skip when: - it is a PR (we never want to skip on nightly tests); and + # - no danger files are changed; and + # - no system test code is changed; and + # - NOT (source code is changed AND not only test files) + skip: &skip_system_test >- + $CIRRUS_PR != '' && + !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && + !changesInclude('test/system/**') && + !(changesInclude('**/*.go', '**/*.c') && !changesIncludeOnly('test/**', 'pkg/machine/e2e/**')) depends_on: *build matrix: *platform_axis gce_instance: *standardvm @@ -866,6 +896,7 @@ local_system_test_aarch64_task: &local_system_test_task_aarch64 # Don't create task for tags, or if using [CI:DOCS], [CI:BUILD] # Docs: ./contrib/cirrus/CIModes.md only_if: *not_tag_magic + skip: *skip_system_test depends_on: *build persistent_worker: *mac_pw ec2_instance: *standard_build_ec2_aarch64 @@ -917,6 +948,7 @@ rootless_system_test_task: alias: rootless_system_test # Docs: ./contrib/cirrus/CIModes.md only_if: *not_tag_magic + skip: *skip_system_test depends_on: *build matrix: *platform_axis gce_instance: *standardvm @@ -968,6 +1000,13 @@ buildah_bud_test_task: alias: buildah_bud_test # Docs: ./contrib/cirrus/CIModes.md only_if: *not_tag_magic + # skip when: - it is a PR (we never want to skip on nightly tests); and + # - no danger files are changed; and + # - no build source files are changed and no bud tests + skip: >- + $CIRRUS_PR != '' && + !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && + !changesInclude('**/*build*.go', 'test/buildah-bud/**') depends_on: *build env: <<: *stdenvars diff --git a/contrib/cirrus/cirrus_yaml_test.py b/contrib/cirrus/cirrus_yaml_test.py index f6de553c2a..5ab43ce5d9 100755 --- a/contrib/cirrus/cirrus_yaml_test.py +++ b/contrib/cirrus/cirrus_yaml_test.py @@ -60,6 +60,21 @@ class TestDependsOn(TestCaseBase): msg=('No success aggregation task depends_on "{0}"'.format(task_name)) self.assertIn(task_name, success_deps, msg=msg) + def test_skips(self): + """2024-06 PR#23030: ugly but necessary duplication in skip conditions. Prevent typos or unwanted changes.""" + beginning = "$CIRRUS_PR != '' && !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && " + real_source_changes = " && !(changesInclude('**/*.go', '**/*.c') && !changesIncludeOnly('test/**', 'pkg/machine/e2e/**'))" + + for task_name in self.ALL_TASK_NAMES: + task = self.CIRRUS_YAML[task_name + '_task'] + if 'skip' in task: + skip = task['skip'] + if 'changesInclude' in skip: + msg = ('{0}: invalid skip'.format(task_name)) + self.assertEqual(skip[:len(beginning)], beginning, msg=msg+": beginning part is wrong") + if 'changesIncludeOnly' in skip: + self.assertEqual(skip[len(skip)-len(real_source_changes):], real_source_changes, msg=msg+": changesIncludeOnly() part is wrong") + def not_task(self): """Ensure no task is named 'task'""" self.assertNotIn('task', self.ALL_TASK_NAMES) From f134ab77bcffed9d7dad6ec9695f1ba60ca21c2c Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 20 Jun 2024 19:06:59 +0200 Subject: [PATCH 2/2] cirrus.yml: add CI:ALL mode to force all tests Now that we have source based skips there might be a case where we have to run all tests. One option is to simply change a line in one of the danger files but having something that can be set as title might be easier for users. Signed-off-by: Paul Holzinger --- .cirrus.yml | 8 ++++++++ contrib/cirrus/CIModes.md | 4 ++++ contrib/cirrus/cirrus_yaml_test.py | 2 +- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/.cirrus.yml b/.cirrus.yml index d58e74d48c..9764ada9aa 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -640,11 +640,13 @@ local_integration_test_task: &local_integration_test_task # Docs: ./contrib/cirrus/CIModes.md only_if: *not_tag_branch_build_docs_machine # skip when: - it is a PR (we never want to skip on nightly tests); and + # - CI:ALL title is not set; and # - no danger files are changed; and # - when no int test code is changed; and # - NOT (source code is changed AND NOT only test files) skip: &skip_int_test >- $CIRRUS_PR != '' && + $CIRRUS_CHANGE_TITLE !=~ '.*CI:ALL.*' && !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && !changesInclude('test/e2e/**', 'test/utils/**') && !(changesInclude('**/*.go', '**/*.c') && !changesIncludeOnly('test/**', 'pkg/machine/e2e/**')) @@ -743,10 +745,12 @@ podman_machine_task: $CIRRUS_CHANGE_TITLE !=~ '.*CI:BUILD.*' ) || $CIRRUS_CRON == "main" # skip when: - it is a PR (we never want to skip on nightly tests); and + # - CI:ALL title is not set; and # - no danger files are changed; and # - no machine code files are changed skip: &skip_machine_test >- $CIRRUS_PR != '' && + $CIRRUS_CHANGE_TITLE !=~ '.*CI:ALL.*' && !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && !changesInclude('cmd/podman/machine/**', 'pkg/machine/**', '**/*machine*.go') depends_on: *build @@ -871,11 +875,13 @@ local_system_test_task: &local_system_test_task $CIRRUS_CHANGE_TITLE !=~ '.*CI:BUILD.*' && $CIRRUS_CHANGE_TITLE !=~ '.*CI:MACHINE.*' # skip when: - it is a PR (we never want to skip on nightly tests); and + # - CI:ALL title is not set; and # - no danger files are changed; and # - no system test code is changed; and # - NOT (source code is changed AND not only test files) skip: &skip_system_test >- $CIRRUS_PR != '' && + $CIRRUS_CHANGE_TITLE !=~ '.*CI:ALL.*' && !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && !changesInclude('test/system/**') && !(changesInclude('**/*.go', '**/*.c') && !changesIncludeOnly('test/**', 'pkg/machine/e2e/**')) @@ -1001,10 +1007,12 @@ buildah_bud_test_task: # Docs: ./contrib/cirrus/CIModes.md only_if: *not_tag_magic # skip when: - it is a PR (we never want to skip on nightly tests); and + # - CI:ALL title is not set; and # - no danger files are changed; and # - no build source files are changed and no bud tests skip: >- $CIRRUS_PR != '' && + $CIRRUS_CHANGE_TITLE !=~ '.*CI:ALL.*' && !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && !changesInclude('**/*build*.go', 'test/buildah-bud/**') depends_on: *build diff --git a/contrib/cirrus/CIModes.md b/contrib/cirrus/CIModes.md index 409334ab73..0156de4695 100644 --- a/contrib/cirrus/CIModes.md +++ b/contrib/cirrus/CIModes.md @@ -126,6 +126,10 @@ is removed. commit-change before Cirrus-CI will notice the draft-status update (i.e. pressing the re-run button **is not** good enough). +### Intended `[CI:ALL]` behavior: + +Run even the tasks that are skipped based on changed sources conditions otherwise. + ### Intended Branch tasks (and Cirrus-cron jobs): + *build* + swagger diff --git a/contrib/cirrus/cirrus_yaml_test.py b/contrib/cirrus/cirrus_yaml_test.py index 5ab43ce5d9..cf62097e4f 100755 --- a/contrib/cirrus/cirrus_yaml_test.py +++ b/contrib/cirrus/cirrus_yaml_test.py @@ -62,7 +62,7 @@ class TestDependsOn(TestCaseBase): def test_skips(self): """2024-06 PR#23030: ugly but necessary duplication in skip conditions. Prevent typos or unwanted changes.""" - beginning = "$CIRRUS_PR != '' && !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && " + beginning = "$CIRRUS_PR != '' && $CIRRUS_CHANGE_TITLE !=~ '.*CI:ALL.*' && !changesInclude('.cirrus.yml', 'Makefile', 'contrib/cirrus/**', 'vendor/**', 'hack/**', 'version/rawversion/*') && " real_source_changes = " && !(changesInclude('**/*.go', '**/*.c') && !changesIncludeOnly('test/**', 'pkg/machine/e2e/**'))" for task_name in self.ALL_TASK_NAMES: