Skip to content

ci: fix shellcheck findings and trim workflow comments (#1967) - #2023

Merged
JarryShaw merged 1 commit into
mainfrom
ci/1967-step5-shellcheck-trim
Oct 22, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
ci/1967-step5-shellcheck-trim

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 21, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

Step 5, part 1 of #1967, over every workflow except unit-tests.yml and experimental.yml (#2006, #2021 and #2022 edit those).

  • Shellcheck (owner's option 1): every finding outside unit-tests.yml fixed at its root — quoted '.[extras]' (SC2102), one quoted { ...; } >> "$GITHUB_OUTPUT" group (SC2129/SC2086), exit "$status", find ... -exec isort ... {} + for the unquoted $(find) (SC2046). No disables. actionlint 1.7.12 now reports only unit-tests.yml's fixture-tier step (SC2086), left for part 2.
  • Comments: restatements, history narration, duplicated rationale, CodeQL boilerplate and dead commented-out keys cut; each non-obvious why and its #NNN kept. Parsed YAML is unchanged apart from the shellcheck edits.
  • Citations (decision A): releasing.rst and workflows.rst cite create-release.yml, python-compatibility.yml and coverage-comment.yml by job and step name; a new test rejects line-number citations of them, replacing the line-anchor table.

Tests: the tests/project modules reading these workflows and docs pass (1112 passed, 5 skipped). make test not run. Refs #1967.

@JarryShaw JarryShaw added ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 21, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 21, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 6f0b8dcde.

A Sonnet cross-review confirmed:

  • Parsed YAML: across the 12 workflows it differs only in run: strings, and 7 files have comment-only edits.
  • Shell edits:
    • Each $GITHUB_OUTPUT group writes the same keys in the same order.
    • exit "$status" is quoted.
    • find -exec isort {} + keeps the same file set and flags, and still fails when isort fails.
  • Comments: all 36 #NNN citations in the removed comment lines are still present.
  • Removed commented-out create-release keys: referenced nowhere.
  • Doc citations: each step named resolves on the head. The new by-name test fails on main's docs (6) and passes here.
  • Linters: actionlint 1.7.12 (checksum verified) leaves only the known unit-tests.yml SC2086.
  • Tests: 618 workflow-related tests/project tests pass.

Two calls for you:

  1. Removed keys: the dead commented-out keys are gone from create-release.
  2. isort: an empty find match is now silent rather than an error. Both roots always contain modules, so this is judged acceptable.

Conflicts: one workflows.rst paragraph conflicts with #2006 and #2022 (around the required-checks and Compat text). The second PR to land rebases by keeping this PR's wording.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 21, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

resolve conflicts

@JarryShaw
JarryShaw force-pushed the ci/1967-step5-shellcheck-trim branch from 6f0b8dc to 4ae58a4 Compare October 21, 2026 21:45
@JarryShaw

Copy link
Copy Markdown
Owner Author

Rebased onto main dbfa54b20 and resolved the conflict. The new head is 4ae58a4d2, and the PR is mergeable.

The only conflict was in workflows.rst, in the required-checks / Compat paragraph. I kept this PR's by-name wording together with main's unit-tests.yml:1937, which is name: Required checks passed. The range-diff against 6f0b8dcde changes only those unit-tests.yml citations: each moved +33 to match #2021, for example 1738-1813 to 1771-1846. The rest of the patch is unchanged, so GOOD TO GO carries over.

Locally, 48 workflow and docs pin modules gave 1120 passed. actionlint still flags only the unit-tests.yml SC2086, which has moved to line 1338.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Project tests failed again.

@JarryShaw

Copy link
Copy Markdown
Owner Author

That run was on 4ae58a4d2, which didn't yet have #2022. It hit the same three uutils timeout tests on an Ubuntu 26.04 runner as main did (#2020): test_a_timeout_exits_124, ..._records_137_and_its_time and test_the_timeout_ends_the_run_in_progress_and_starts_no_other. This PR doesn't touch them. Your branch update to aa8d16551 brings in #2022's fix. Locally, the timeout, AppArmor and queue pins pass on that tree (96 passed), and the new CI run should come back green.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 91.54% (unit tier, Python 3.15, b813b239a, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19597 881 2618 867 92.11%
pcapkit/corekit 2567 71 816 37 96.57%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 2930 23 1022 25 98.79%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17436 219 4582 185 98.15%
pcapkit/toolkit 872 36 298 10 95.21%
pcapkit/utilities 529 7 160 5 98.26%
pcapkit/vendor 4431 1995 1018 167 51.73%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Ordering check for this PR, since the "Plain unittest ordering" job is skipped on PRs: on main 185b3176f with this PR merged in, python util/run_unittest_leg.py project gave Ran 1675 tests in 483s, OK (skipped=71), exit 0. Project is the only ordering leg that this PR's test files feed. The KTLO legs report 0 failing test IDs across 13 legs. The only check still running is the experimental GraalPy leg.

@JarryShaw

Copy link
Copy Markdown
Owner Author

resolve conflicts

Step 5, part 1 of #1967, over every workflow except unit-tests.yml and
experimental.yml (open PRs edit those):

- Fix actionlint's shellcheck findings at the root: quote the `.[extras]`
  pip targets (SC2102), group `$GITHUB_OUTPUT` writes into one quoted
  redirect (SC2129, SC2086), quote `exit "$status"` (SC2086), and run
  isort through `find ... -exec ... {} +` instead of an unquoted
  `$(find)` (SC2046).
- Trim comments: drop restatements of the YAML, history narration,
  duplicated rationale, CodeQL template boilerplate and dead
  commented-out keys; keep each non-obvious why and its issue ref.
- Cite create-release.yml, python-compatibility.yml and
  coverage-comment.yml by job and step name in releasing.rst and
  workflows.rst (decision A), and replace the line-anchor test with one
  that rejects line-number citations of those files.
- Update the conda pin test for the quoted `'.[all]'`.

actionlint 1.7.12 now reports nothing outside unit-tests.yml.
@JarryShaw
JarryShaw force-pushed the ci/1967-step5-shellcheck-trim branch from aa8d165 to b813b23 Compare October 22, 2026 02:06
@JarryShaw

Copy link
Copy Markdown
Owner Author

Rebased onto main 609bd9370 (#2006's squash) as b813b239a. The only conflict was workflows.rst: I kept main's unit-tests.yml:1964 citation and this PR's by-name wording for the compatibility job. range-diff shows this PR's own hunks unchanged; only context lines moved. The tests that read the touched docs and workflows pass on the rebased tree (466 passed, 4 skipped). The project ordering leg is re-running there now.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Ordering check on b813b239a: python util/run_unittest_leg.py project on this tree gave Ran 1685 tests in 533s, OK (skipped=71), exit 0.

@JarryShaw
JarryShaw merged commit 8d9b69e into main Oct 22, 2026
59 checks passed
@JarryShaw
JarryShaw deleted the ci/1967-step5-shellcheck-trim branch October 22, 2026 02:40
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 22, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 22, 2026
JarryShaw added a commit that referenced this pull request Oct 22, 2026
- lint.yml: drop the actionlint step's `continue-on-error`, so a finding
  fails Lint; move it below the advisory steps and run it on `!cancelled()`
  so a finding skips none of them and a failed setup does not skip it.
  Version and sha256 pin unchanged.
- lint.yml: the step's comment says it blocks, why that is safe now
  (#2023, #2037, #2038), and that the image's shellcheck can drift.
- workflows.rst: Workflow Lint no longer calls the step advisory.
- test_actionlint_in_lint_1967: TestItBlocks pins the blocking shape;
  test_unit_tests_shellcheck_1967's docstring no longer says advisory.
JarryShaw added a commit that referenced this pull request Oct 22, 2026
- lint.yml: drop the actionlint step's `continue-on-error`, so a finding
  fails Lint; move it below the advisory steps and run it on `!cancelled()`
  so a finding skips none of them and a failed setup does not skip it.
  Version and sha256 pin unchanged.
- lint.yml: the step's comment says it blocks, why that is safe now
  (#2023, #2037, #2038), and that the image's shellcheck can drift.
- workflows.rst: Workflow Lint no longer calls the step advisory.
- test_actionlint_in_lint_1967: TestItBlocks pins the blocking shape;
  test_unit_tests_shellcheck_1967's docstring no longer says advisory.
JarryShaw added a commit that referenced this pull request Oct 22, 2026
- lint.yml: drop the actionlint step's `continue-on-error`, so a finding
  fails Lint; move it below the advisory steps and run it on `!cancelled()`
  so a finding skips none of them and a failed setup does not skip it.
  Version and sha256 pin unchanged.
- lint.yml: the step's comment says it blocks, why that is safe now
  (#2023, #2037, #2038), and that the image's shellcheck can drift.
- workflows.rst: Workflow Lint no longer calls the step advisory.
- test_actionlint_in_lint_1967: TestItBlocks pins the blocking shape;
  test_unit_tests_shellcheck_1967's docstring no longer says advisory.
JarryShaw added a commit that referenced this pull request Oct 22, 2026
- lint.yml: drop the actionlint step's `continue-on-error`, so a finding
  fails Lint; move it below the advisory steps and run it on `!cancelled()`
  so a finding skips none of them and a failed setup does not skip it.
  Version and sha256 pin unchanged.
- lint.yml: the step's comment says it blocks, why that is safe now
  (#2023, #2037, #2038), and that the image's shellcheck can drift.
- workflows.rst: Workflow Lint no longer calls the step advisory.
- test_actionlint_in_lint_1967: TestItBlocks pins the blocking shape;
  test_unit_tests_shellcheck_1967's docstring no longer says advisory.
JarryShaw added a commit that referenced this pull request Oct 22, 2026
- lint.yml: drop the actionlint step's `continue-on-error`, so a finding
  fails Lint; move it below the advisory steps and run it on `!cancelled()`
  so a finding skips none of them and a failed setup does not skip it.
  Version and sha256 pin unchanged.
- lint.yml: the step's comment says it blocks, why that is safe now
  (#2023, #2037, #2038), and that the image's shellcheck can drift.
- workflows.rst: Workflow Lint no longer calls the step advisory.
- test_actionlint_in_lint_1967: TestItBlocks pins the blocking shape;
  test_unit_tests_shellcheck_1967's docstring no longer says advisory.
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant