ci: skip build & test jobs for .md-only PRs#1508
Conversation
…ork_filter.sh Adds pytest-based functional tests for pod network filter scenario as specified in issue krkn-chaos#1434. Covers: Happy-path - TCP port block (egress) - UDP port block - Multi-protocol (TCP+UDP) - Multi-port - Ingress direction - Both directions - Label selector targeting - Duration & cleanup logging Negative - Non-existent target pod - No matching pods - Invalid config structure Cleanup - Filter rules removed - Helper pod deleted Closes krkn-chaos#1434 Signed-off-by: Mas Jono <masjonodev@gmail.com>
Add paths-ignore filter to both tests and tests_v2 workflows so PRs that only touch .md files don't trigger full CI. Closes krkn-chaos#1447 Signed-off-by: Mas Jono <masjonodev@gmail.com>
|
Thanks for your contribution! However, this PR is linked to issue #1447, which is not assigned to you. Please refer to our contribution guidelines. PRs should only be opened by contributors who are assigned to the linked issue. |
|
👀 Human Input Needed → Pair Review Briefing |
| on: | ||
| pull_request: | ||
| paths-ignore: | ||
| - '**.md' |
There was a problem hiding this comment.
Adding paths-ignore: ['**.md'] to a workflow whose emitted check-run is a required status check (Functional & Unit Tests here, Tests v2 (pytest functional) in the sibling file) makes docs-only PRs unmergeable — skipped workflows never report a status, so the required check stays pending forever. Consider adding a companion no-op bypass workflow that runs on paths: ['**.md'] and emits check-runs with matching job names (Functional & Unit Tests, Tests v2 (pytest functional)) so branch protection is satisfied; see PR #1471 for the pattern the repo has previously adopted. Other locations where this applies: .github/workflows/tests_v2.yml line 5.
Severity: high
🤖 Was this useful? React with 👍 or 👎
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
There was a problem hiding this comment.
build_config and run_kraken in CI/tests_v2/lib/kraken.py are pytest fixtures (@pytest.fixture) that must be requested via test-function parameters; importing them at module scope and calling them directly (e.g. build_config(base_yaml=..., overrides=..., namespace=...), run_kraken(cfg, repo_root=repo_root) in _run_filter) raises Failed: fixture ... called directly and also uses signatures that don't exist — the real build_config factory takes (scenario_type, scenario_file, filename) and returns a config-file path, and the run_kraken fixture returns a callable that takes (config_path, timeout, extra_args) and returns CompletedProcess, not the (rc, out, err) 3-tuple this file unpacks. As written, no test in this module can execute.
Severity: high
🤖 Was this useful? React with 👍 or 👎
| "-l", "app.kubernetes.io/created-by=krkn", | ||
| "-o", "name", | ||
| ) | ||
| assert "krkn-network" not in result, "Helper pod leaked after cleanup" |
There was a problem hiding this comment.
The kubectl fixture in CI/tests_v2/lib/k8s.py returns a CompletedProcess, not a string, so "krkn-network" not in result raises TypeError: argument of type 'CompletedProcess' is not iterable; the intended substring check needs result.stdout (and would also want to assert on result.returncode).
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| network_chaos_ng_scenarios: | ||
| - id: "pod_network_filter" | ||
| label_selector: "app=network-filter-test" | ||
| namespace: "krkn-filter-test" |
There was a problem hiding this comment.
The scenario hardcodes namespace: "krkn-filter-test" while the tests pass the ephemeral test_namespace fixture value (krkn-test-<uuid>) into the scenario, so the two disagree, and no test in this module ever deploys resource.yaml (there is no deploy_workload, k8s_core.create_namespace, or kubectl("apply", "-f", ...) call) — even with the fixture wiring fixed, the scenario would target a non-existent namespace and pod. Existing v2 scenarios apply their workload via the shared deploy_workload fixture and patch namespace via the scenario helper — consider following that pattern.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| node_scenarios: marks a test as a node chaos scenario test (node reboot/stop-start) | ||
| node_network_chaos: marks a test as a node network chaos scenario test | ||
| namespace_deletion: marks a test as a namespace deletion (service_disruption) scenario test | ||
| pod_network_filter: marks a test as a pod network filter scenario test |
There was a problem hiding this comment.
The PR title, description, and linked issue (#1447) describe only the paths-ignore change to two workflow files, but this marker and the ~285-line CI/tests_v2/scenarios/pod_network_filter/* addition come from an unrelated second commit (test_v2: migrate pod_network_filter tests from CI/tests/test_pod_network_filter.sh) that isn't mentioned anywhere in the PR — is this intentional, or should the test-migration commit be split into its own PR (against the tracking issue for that migration, e.g. #1434) so each concern can be reviewed independently?
Severity: low
🤖 Was this useful? React with 👍 or 👎
Description
Adds
paths-ignore: ['**.md']to bothtests.ymlandtests_v2.ymlworkflows so PRs that only modify documentation files skip the full CI pipeline.Closes #1447
Changes
.github/workflows/tests.yml— added paths-ignore for .md files.github/workflows/tests_v2.yml— same change