diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml index faf9f5f..b998776 100644 --- a/.github/actionlint.yaml +++ b/.github/actionlint.yaml @@ -23,3 +23,12 @@ paths: # same actionlint gap, see the comment on the pack-release.yaml entry. - 'property "workflow_repository" is not defined in object type' - 'property "workflow_sha" is not defined in object type' + .github/workflows/sync-project-priority.yaml: + ignore: + # Every GraphQL query in this workflow is single-quoted on purpose: the + # $p/$i/$f/$o inside are GraphQL variable references that must reach the + # API literally, not shell variables. Their values are passed separately + # via `gh api graphql -F/-f`. shellcheck cannot tell the two kinds of `$` + # apart, so each query body trips SC2016. Drop this entry if the queries + # ever stop using GraphQL variables. + - "shellcheck reported issue in this script: SC2016:.+" diff --git a/.github/scripts/test_sync_project_priority.py b/.github/scripts/test_sync_project_priority.py new file mode 100644 index 0000000..923eda4 --- /dev/null +++ b/.github/scripts/test_sync_project_priority.py @@ -0,0 +1,214 @@ +"""Tests for the "Apply priority from labels" step of sync-project-priority.yaml. + +The step is an inline bash script, so these tests extract that exact script out +of the workflow YAML and run it under bash with a stub `gh` on PATH. Testing the +shipped text rather than a copy means the tests cannot drift away from what +actually runs in CI. + +Run with: python -m pytest .github/scripts/test_sync_project_priority.py -v +""" +import json +import os +import subprocess +from pathlib import Path + +import pytest +from ruamel.yaml import YAML + +WORKFLOW = ( + Path(__file__).resolve().parents[1] / "workflows" / "sync-project-priority.yaml" +) +STEP_NAME = "Apply priority from labels" + +# Mirrors the Priority single-select options a board is expected to carry. +OPTIONS = [ + {"id": "opt-critical", "name": "Critical"}, + {"id": "opt-high", "name": "High"}, + {"id": "opt-medium", "name": "Medium"}, + {"id": "opt-low", "name": "Low"}, + {"id": "opt-icebox", "name": "Icebox"}, +] + +# Label names carry a trailing emoji in the Nebari repos; the step is expected to +# read the priority out of the "priority: " prefix and ignore the rest. +HIGH = "priority: high \U0001f525" +LOW = "priority: low \U0001f331" +DISCUSSION = "needs: discussion \U0001f4ac" +AREA_CI = "area: ci \U0001f527" +# Labels that contain "priority: " but do not start with it. The step anchors +# its match to the start of the label name, so neither is a priority label. +PHRASE = "blocked by priority: high \U0001f525" +HYPHENATED = "de-priority: high \U0001f525" + +# A stub `gh` that records every invocation as one JSON line and answers with a +# response shaped like the real GraphQL API, so the step's `--jq` still works. +GH_STUB = """#!/usr/bin/env python3 +import json, os, sys +with open(os.environ["GH_CALLS"], "a") as fh: + fh.write(json.dumps(sys.argv[1:]) + "\\n") +print(json.dumps({"data": {"clearProjectV2ItemFieldValue": {"projectV2Item": {"id": "item"}}, + "updateProjectV2ItemFieldValue": {"projectV2Item": {"id": "item"}}}})) +""" + + +def _step_script(): + yaml = YAML() + with open(WORKFLOW) as fh: + doc = yaml.load(fh) + for step in doc["jobs"]["sync"]["steps"]: + if step.get("name") == STEP_NAME: + return step["run"] + raise AssertionError(f"step {STEP_NAME!r} not found in {WORKFLOW}") + + +def _step_env(): + yaml = YAML() + with open(WORKFLOW) as fh: + doc = yaml.load(fh) + for step in doc["jobs"]["sync"]["steps"]: + if step.get("name") == STEP_NAME: + return dict(step.get("env") or {}) + raise AssertionError(f"step {STEP_NAME!r} not found in {WORKFLOW}") + + +@pytest.fixture +def run_step(tmp_path): + """Run the real step script and report which GraphQL mutations it issued.""" + bindir = tmp_path / "bin" + bindir.mkdir() + gh = bindir / "gh" + gh.write_text(GH_STUB) + gh.chmod(0o755) + + runner_temp = tmp_path / "runner" + runner_temp.mkdir() + calls = tmp_path / "calls.jsonl" + + def _run(labels, *, action, changed_label, is_issue=True, options=OPTIONS): + (runner_temp / "options.json").write_text(json.dumps(options)) + script = tmp_path / "step.sh" + script.write_text(_step_script()) + + env = { + **os.environ, + "PATH": f"{bindir}:{os.environ['PATH']}", + "GH_CALLS": str(calls), + "GH_TOKEN": "stub", + "RUNNER_TEMP": str(runner_temp), + "PROJECT_ID": "PVT_project", + "FIELD_ID": "PVTSSF_field", + "ITEM_ID": "PVTI_item", + "LABELS": json.dumps([{"name": name} for name in labels]), + "IS_ISSUE": "true" if is_issue else "false", + "EVENT_ACTION": action, + "CHANGED_LABEL": changed_label, + } + proc = subprocess.run( + ["bash", str(script)], env=env, capture_output=True, text=True + ) + assert proc.returncode == 0, f"step failed: {proc.stdout}\n{proc.stderr}" + + mutations, option_ids = [], [] + if calls.exists(): + for line in calls.read_text().splitlines(): + argv = json.loads(line) + blob = " ".join(argv) + if "clearProjectV2ItemFieldValue" in blob: + mutations.append("clear") + elif "updateProjectV2ItemFieldValue" in blob: + mutations.append("update") + option_ids += [ + a.split("=", 1)[1] for a in argv if a.startswith("o=") + ] + calls.unlink() + return mutations, option_ids, proc.stdout + + return _run + + +# (case id, labels on the item, event action, label added/removed, expected mutations) +CASES = [ + # An issue that never had a priority label must not be touched, whatever + # unrelated label activity happens on it. This is the reported bug. + ("opened_no_labels", [], "opened", "", []), + ("opened_only_other_labels", [DISCUSSION], "opened", "", []), + ("unrelated_label_added", [AREA_CI], "labeled", AREA_CI, []), + ("unrelated_label_added_beside_others", [DISCUSSION, AREA_CI], "labeled", AREA_CI, []), + ("unrelated_label_removed", [AREA_CI], "unlabeled", DISCUSSION, []), + # Removing the priority label is the one case that may clear the field. + ("priority_label_removed", [DISCUSSION], "unlabeled", HIGH, ["clear"]), + ("last_priority_label_removed_no_labels_left", [], "unlabeled", LOW, ["clear"]), + # Setting still works, and wins over clearing when a priority label remains. + ("priority_label_added", [HIGH], "labeled", HIGH, ["update"]), + ("priority_label_present_on_open", [HIGH, DISCUSSION], "opened", "", ["update"]), + ("one_of_two_priority_labels_removed", [LOW], "unlabeled", HIGH, ["update"]), + ("unrelated_label_added_with_priority_present", [HIGH, AREA_CI], "labeled", AREA_CI, ["update"]), + # Applying several labels at once fires several concurrent runs, each + # carrying its own snapshot of the label set. A run whose snapshot has not + # caught up with a priority label that was just added must not read that as + # "no priority" and clear the field. + ("stale_snapshot_on_priority_label_added", [], "labeled", HIGH, []), + # Only the exact "priority: " prefix counts. + ("lookalike_label_removed", [], "unlabeled", "prioritisation notes", []), + ("prefix_without_separator_removed", [], "unlabeled", "priority:high", []), + # ...and it has to be a prefix. A label that merely contains "priority: " + # somewhere inside it is an ordinary label, so it neither sets the field + # nor authorises a clear. + ("priority_text_inside_phrase_added", [PHRASE], "labeled", PHRASE, []), + ("hyphenated_priority_text_added", [HYPHENATED], "labeled", HYPHENATED, []), + ("priority_text_inside_phrase_removed", [], "unlabeled", PHRASE, []), +] + + +@pytest.mark.parametrize( + "case_id,labels,action,changed_label,expected", + CASES, + ids=[c[0] for c in CASES], +) +def test_mutations_issued(run_step, case_id, labels, action, changed_label, expected): + mutations, _, stdout = run_step(labels, action=action, changed_label=changed_label) + assert mutations == expected, stdout + + +def test_priority_label_maps_to_matching_option(run_step): + _, option_ids, stdout = run_step([HIGH], action="labeled", changed_label=HIGH) + assert option_ids == ["opt-high"], stdout + + +def test_pull_requests_are_never_cleared(run_step): + # clearProjectV2ItemFieldValue only supports issues. + mutations, _, stdout = run_step( + [], action="unlabeled", changed_label=HIGH, is_issue=False + ) + assert mutations == [], stdout + + +def test_board_without_options_issues_no_update(run_step): + # A Priority field with no single-select options cannot be set; the step + # must skip rather than send a mutation with an empty option id. + mutations, _, stdout = run_step( + [HIGH], action="labeled", changed_label=HIGH, options=[] + ) + assert mutations == [], stdout + + +def test_unknown_priority_value_issues_no_mutation(run_step): + mutations, _, stdout = run_step( + ["priority: bogus"], action="labeled", changed_label="priority: bogus" + ) + assert mutations == [], stdout + + +def test_missing_event_context_does_not_clear(run_step, tmp_path): + # If the event carries no label context at all the step must take the safe + # branch, not abort under `set -u` and not clear. + mutations, _, stdout = run_step([], action="", changed_label="") + assert mutations == [], stdout + + +def test_step_env_wires_the_event_context(): + # The shell logic above is only correct if the workflow actually passes the + # event action and the changed label into the step. + env = _step_env() + assert "github.event.action" in str(env.get("EVENT_ACTION", "")) + assert "github.event.label.name" in str(env.get("CHANGED_LABEL", "")) diff --git a/.github/workflows/lint-test.yaml b/.github/workflows/lint-test.yaml index 7040ff4..3fef30b 100644 --- a/.github/workflows/lint-test.yaml +++ b/.github/workflows/lint-test.yaml @@ -2,14 +2,10 @@ name: lint-test # CI for this repo's own reusable workflows and helper scripts. # -# Scoped to the workflows introduced/maintained under this effort -# (pack-build-image.yaml, pack-release.yaml, sign-smoke.yaml, and this -# file) rather than the whole .github/workflows/ directory: the other -# pre-existing workflows here (sync-issue-templates.yaml, sync-project-priority.yaml) -# already have unrelated actionlint findings (a floating, EOL -# actions/checkout@v3 pin and a few shellcheck info-level notes) that -# predate this change and are out of scope for it. Widen the file list -# below once those are cleaned up separately. +# An explicit file list rather than the whole .github/workflows/ directory: +# sync-issue-templates.yaml still carries a pre-existing floating, EOL +# actions/checkout@v3 pin that is out of scope here. Add it below once that +# is cleaned up. on: pull_request: @@ -34,7 +30,8 @@ jobs: .github/workflows/pack-build-image.yaml \ .github/workflows/pack-release.yaml \ .github/workflows/lint-test.yaml \ - .github/workflows/sign-smoke.yaml + .github/workflows/sign-smoke.yaml \ + .github/workflows/sync-project-priority.yaml pytest: name: pytest diff --git a/.github/workflows/sync-project-priority.yaml b/.github/workflows/sync-project-priority.yaml index 8f0fb53..d20229b 100644 --- a/.github/workflows/sync-project-priority.yaml +++ b/.github/workflows/sync-project-priority.yaml @@ -3,6 +3,13 @@ name: Sync project priority from labels # Reusable workflow: adds an issue or PR to a Projects v2 board and syncs the # Priority single-select field based on labels of the form "priority: ". # +# The sync is deliberately asymmetric. A "priority: " label sets the +# field on any event, but the field is cleared only when an "unlabeled" event +# removes a "priority: " label and none remains on the item. The field +# is also set by hand on the board, so treating "this item carries no priority +# label" as "clear it" would wipe a manual value every time an unrelated label +# was added or removed. +# # Triggered by caller repos on issues and pull_request_target events # (opened, labeled, unlabeled). The caller passes the project's GraphQL node ID # and a PAT with project: write scope. @@ -93,17 +100,35 @@ jobs: ITEM_ID: ${{ steps.add.outputs.item_id }} LABELS: ${{ toJSON(github.event.issue.labels || github.event.pull_request.labels) }} IS_ISSUE: ${{ github.event.issue.node_id != '' && github.event.issue.node_id != null }} + EVENT_ACTION: ${{ github.event.action }} + CHANGED_LABEL: ${{ github.event.label.name }} run: | set -uo pipefail PRIORITY=$(echo "$LABELS" | jq -r '[.[].name | capture("^priority: (?

[a-zA-Z]+)") | .p][0] // ""') if [ -z "$PRIORITY" ]; then + # The Priority field is also set by hand on the board, so "this item + # has no priority label" is not on its own a reason to clear it. + # Only the removal of a priority label is. Without this guard every + # opened/labeled/unlabeled event on an unprioritised item wipes the + # field, including events about labels that have nothing to do with + # priority. + # Defaulted rather than bare: github.event.label is absent on + # "opened", and under `set -u` a bare expansion would abort the step + # instead of taking the safe branch. + case "${EVENT_ACTION:-}/${CHANGED_LABEL:-}" in + "unlabeled/priority: "*) ;; + *) + echo "No priority label, and no priority label was removed; leaving Priority untouched." + exit 0 + ;; + esac # clearProjectV2ItemFieldValue only supports issues, not pull requests. if [ "$IS_ISSUE" != "true" ]; then - echo "No priority label; skipping clear (only issues support clearing fields)." + echo "Priority label removed; skipping clear (only issues support clearing fields)." exit 0 fi - echo "No priority label; clearing field." + echo "Priority label removed and none remain; clearing field." gh api graphql -f query=' mutation($p:ID!,$i:ID!,$f:ID!){ clearProjectV2ItemFieldValue(input:{projectId:$p,itemId:$i,fieldId:$f}){ projectV2Item { id } } diff --git a/README.md b/README.md index f11b1cf..9d2ad1a 100644 --- a/README.md +++ b/README.md @@ -38,6 +38,9 @@ Below is a quick list of what you'll find in this repository: > **Note** > For the PR trigger, callers must use `pull_request_target`, **not** `pull_request`. PRs opened from forks do not receive secrets under `pull_request`, so the PAT is empty and the job fails. `pull_request_target` runs in the base repo's context, making the secret available. This is safe because the workflow checks out no PR code — it only reads trusted event metadata and calls the GitHub API. + > **Note** + > Setting and clearing are not symmetric. A `priority: ` label sets the field on any event the caller subscribes to, but the field is cleared only when an `unlabeled` event removes a `priority: ` label and no priority label remains on the item. Every other event leaves the field alone. This matters because the `Priority` field is also set by hand on the board: treating "this item carries no priority label" as "clear it" would wipe a manual value whenever an unrelated label was added or removed. + > **Warning** > The syncing action requires a Personal Authentication Token (PAT) which is currently set up through [Nebari-sensei](https://github.com/nebari-sensei)