Skip to content

feat: add advisory linter compatibility checks to CI and release - #920

Open
Hangyi (HangyiWang) wants to merge 2 commits into
release/1.1.0-previewfrom
users/hangyiwang/index-compatibility-check
Open

Hangyi (HangyiWang) wants to merge 2 commits into
release/1.1.0-previewfrom
users/hangyiwang/index-compatibility-check

Conversation

@HangyiWang

@HangyiWang Hangyi (HangyiWang) commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

When we add entries to our linter_exclusions.yml without updating the corresponding exclusions in Azure/azure-cli-extensions, extension-index CI can fail even when our own linter passes.

This PR adds an advisory compatibility check to PR CI and the release workflow. It highlights lint findings and local-only exclusions so we can identify whether a separate upstream PR is needed before merging or releasing.

The check remains non-blocking: HIGH findings and tool failures show red, while MEDIUM-only findings produce warnings. Existing required checks and release approval gates are unchanged.

Check the candidate wheel with merged index exclusions, report provenance and lint findings, and keep release approval dependencies unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 23:31
@HangyiWang Hangyi (HangyiWang) changed the title Add advisory index compatibility checks to CI and release feat: add advisory linter compatibility checks to CI and release Oct 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Whitespace-sensitive parser guards can incorrectly report incomplete or unrecognized lint failures as a clean pass.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds a shared advisory check for candidate-wheel compatibility with the merged extension index, without changing existing CI or release approval gates.

Changes:

  • Reuses the built wheel in an isolated compatibility workflow.
  • Publishes findings, exclusion differences, logs, and provenance.
  • Adds checker tests and contributor guidance.
File Description
scripts/​check_index_compatibility.py Validates the environment and wheel, runs lint, and produces reports.
CONTRIBUTING.md Documents advisory behavior and release review guidance.
azext_iot/​tests/​test_index_compatibility_unit.py Tests parsing, isolation, reporting, and workflow wiring.
.github/​workflows/​release_workflow.yml Adds the advisory release job.
.github/​workflows/​index_compatibility.yml Defines the shared isolated check and artifact publishing.
.github/​workflows/​ci_workflow.yml Adds the advisory CI job.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/check_index_compatibility.py
@zzcodework

Copy link
Copy Markdown
Contributor

Azure SRE Agent - automated review

Reviewing full diff (head 20711664)

Adds an advisory index-compatibility job to PR CI and the release workflow: it lints the already-built azure-iot-cli-ext wheel in a clean venv against Azure/azure-cli dev and Azure/azure-cli-extensions main, deliberately without this repo's linter_exclusions.yml. The provenance work in scripts/check_index_compatibility.py (verify_wheel, verify_environment, verify_loaded_extension) is unusually thorough for this kind of check — it actually proves the thing being linted is the candidate wheel rather than the source tree, which is the failure mode such jobs normally have. Findings below are all non-blocking.

Blocking

None.

Suggestions

  1. .github/workflows/index_compatibility.yml (~lines 53, 67) + scripts/check_index_compatibility.py check_toolchain (~line 609) — toolchain drift turns this advisory check red on contributors' PRs, with no out-of-band warning and no way to tell it apart from a real regression.

    check_toolchain compares the running interpreter and azdev version against whatever azure-cli-extensions@main declares at that moment (versionSpec from the AzdevLinterModifiedExtensions job, and the azdev== pin in .azure-pipelines/templates/azdev_setup.yml). The running values are hardcoded here — python-version: "3.14" and pip install azdev==0.2.13 — and the second is additionally pinned by test_reusable_workflow_is_isolated_read_only_and_does_not_mask_failures (assert "azdev==0.2.13" in setup["run"]). The instant upstream bumps either value, every open PR in this repo gets a red index-compatibility / Index compatibility, and the only remedy is a workflow edit plus a test edit. The same applies to the structural guard a few lines up, which raises "Upstream azdev/CLI setup changed; review the compatibility workflow." if azdev_setup.yml stops containing -b dev https://github.com/Azure/azure-cli.git or pins azdev more than once.

    CONTRIBUTING.md frames this as "toolchain drift fails explicitly instead of silently claiming parity", which is the right call — the issue is purely where the failure lands. Two cheap mitigations: (a) add a schedule: trigger so drift is detected against dev out-of-band, before it reaches someone's PR; (b) make the drift outcome distinguishable at a glance — set a distinct report["result"] (e.g. "Toolchain drift") rather than the generic "Failed", so the summary and the artifact say "upstream moved" instead of looking identical to "this PR broke a command".

  2. scripts/check_index_compatibility.py parse_lint (~lines 581-600) — the detail-line parser never ends a FAIL block on a non-indented line, so findings can be attributed to the wrong rule.

    current is set by a FAIL RULE match and cleared only by a pass RULE match. Any line that is neither a RULE match nor 4-space-indented — a blank line, a separator, or azdev's trailing custom-pylint section — leaves current armed. The subsequent elif current[0] and line.startswith(" ") and " - " in line: branch then attributes any later indented line containing " - " to the last failing rule, fabricating a finding with the wrong rule and severity. That finding also flows into local_exclusion lookup and into the ::error::/::warning:: annotation, so a HIGH rule can pick up unrelated trailing text and surface it as a defect.

    One line fixes it: clear current in a final else: (any line that is neither a RULE match nor a continuation ends the block). Worth a regression case where a FAIL block is followed by an unindented separator and then an indented pylint line — the existing parametrized cases all keep the FAIL block contiguous, so none of them exercise this.

  3. scripts/check_index_compatibility.py check (~line 742) — on pull_request the recorded source_commit is a SHA that does not exist after the run.

    run(["git", "rev-parse", "HEAD"], source_root) runs in the source checkout, and for pull_request events actions/checkout checks out the ephemeral refs/pull/N/merge commit. So the artifact's provenance table names a synthetic merge SHA that cannot be used to reproduce the report — which undercuts the point of recording cli_commit/index_commit alongside it. Recording ${{ github.event.pull_request.head.sha || github.sha }} as a second provenance field (or passing it in as a flag) makes the report reproducible.

Nits

  1. .github/workflows/index_compatibility.yml (~lines 75, 88) — if the first step ("Initialize isolated paths") ever fails, COMPAT_ROOT is unset, so the summary step runs mkdir -p "/reports" as the unprivileged runner user (fails), and upload-artifact resolves path: to /reports/ and then errors on if-no-files-found: error. The job is correctly red either way; this only costs a confusing pair of secondary errors on top of the real one. Defaulting with ${COMPAT_ROOT:-$RUNNER_TEMP/index-compatibility} in the summary step would keep the real cause on top.

Checked and clean

  • The artifact contract holds on both callers. ci_workflow.yml's build uses ci_build.yml and release_workflow.yml's build uses release_build.yml; both upload name: azure-iot-cli-ext, which is exactly what index_compatibility.yml downloads. This is the usual way a reused-artifact job breaks on one of two paths, and it does not here.
  • All six REQUIRED_RULES survive --min-severity medium — verified directly against the azdev 0.2.13 wheel: missing_group_help, missing_command_help and no_parameter_defaults_for_update_commands are LinterSeverity.HIGH; broken_site_link_from_command_group, require_wait_command_if_no_wait and parameter_should_not_end_in_resource_group are LinterSeverity.MEDIUM. None is LOW, so the severity filter cannot trip the completeness gate in parse_lint and turn the check permanently red.
  • The check really is non-gating for releases. release_workflow.yml's approval needs [security, build, unit-test, azdev_linter, int_test] and draft_github_release needs [approval]; index-compatibility appears in neither, which test_both_callers_reuse_wheel_without_gating_existing_checks_or_release asserts generically rather than by listing names.
  • Exit policy matches the documented one. exit_code = exit_code or int(high) gives: MEDIUM-only with azdev 0 → green; any HIGH → red even when azdev exits 0; a non-zero azdev code preserved verbatim (the (None, 7, 7) parametrize case).
  • The finally block writes report.json and summary.md on every path, including exceptions outside the caught tuple, so the workflow's "Incomplete" fallback is a genuine last resort rather than the normal failure route.
  • test_setup_failure_survives_tee is hermetic. set -euo pipefail plus the stubbed python aborts the brace group at python -m venv, before the two git clone lines, so the unit suite never touches the network — worth noting because a test that executes the real workflow script usually does.
  • azext_iot/tests/test_index_compatibility_unit.py ends in _unit.py, so tox's pytest -k "_unit.py" ./azext_iot/tests selector picks it up; the three shell-executing cases are correctly skipif-gated to Linux.
  • persist-credentials: false on the checkout, permissions: {} at workflow level with contents: read on the job, and no azure/login step — the job executes freshly cloned upstream code with no repo-write token and no Azure credentials, which is the right shape. ci_workflow.yml's caller omits permissions: and inherits the file's contents: read, matching how the sibling linter job is written.
  • CI/tooling-only change, so no HISTORY.rst entry or azext_iot/constants.py VERSION bump is expected.
  • Not restating Copilot's open inline comment on scripts/check_index_compatibility.py line 96. For the record I confirm both sites it names are real: after ANSI.sub the fixture's - \x1b[31m FAIL\x1b[39m becomes - FAIL with two spaces, which the startswith("- FAIL") guard and the ^- FAIL - (?:HIGH|MEDIUM) severity: cross-check both miss, while RULE's \s+ is the only whitespace-tolerant one of the three — so the "a failed rule had no readable findings" safety net is inert against real azdev output.

This is an automated review and may be incomplete.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 02:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The checker’s handling of unreadable linter failures needs human verification.

Review effort: Balanced
Findings: None

Resolved since last review (1)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants