Issue 4940: Add skip deployment flag for PR bot tests

[stuart-bass-cgi]

Resolves #4940

What is being addressed

PR bot /test and /test-extended commands currently always run the full validation flow, including build and deployment jobs, even when the PR validation environment has already been deployed. This makes it slow to re-run smoke or extended E2E tests after transient test failures or test-only changes.

How is this addressed

• Adds a skip_deployment flag for the /test and /test-extended PR bot commands.
• Passes the parsed flag from the PR comment bot into the reusable TRE deployment workflow.
• When skip_deployment is true, skips the deployment/build/register jobs and allows the smoke and requested custom E2E jobs to run against the existing deterministic PR validation environment.
• Keeps the existing external PR SHA validation in place, including when skip_deployment is supplied.
• Updates PR bot command documentation with the new syntax and fork PR examples.
• Adds unit test coverage for the new command parsing behaviour.

[Copilot]

Pull request overview

Adds a skip_deployment flag to PR comment bot /test and /test-extended commands so maintainers can rerun smoke/extended E2E tests against an already-deployed deterministic PR validation environment, avoiding the full build/deploy pipeline when appropriate.

Changes:

• Extended PR bot command parsing to recognize skip_deployment and pass it through to the reusable deployment workflow.
• Updated the reusable workflow to conditionally bypass deployment/build stages and still run smoke/custom E2E jobs.
• Updated developer docs and added unit tests covering the new command parsing behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
docs/tre-developers/github-pr-bot-commands.md	Documents the new skip_deployment flag syntax and fork PR examples for /test and /test-extended.
.github/workflows/pr_comment_bot.yml	Plumbs skipDeployment output from the parsing script into the reusable workflow call.
.github/workflows/deploy_tre_reusable.yml	Adds skipDeployment input and adjusts E2E job conditions to allow running tests when deployment is skipped.
.github/scripts/build.js	Parses skip_deployment, emits skipDeployment output, and updates external-PR SHA extraction to ignore the flag.
.github/scripts/build.test.js	Adds unit tests validating skipDeployment output behavior for internal and external PR command forms.

[stuart-bass-cgi]

@microsoft-github-policy-service agree company="CGI"

[Marcus Robinson (marrobi)]

stuart-bass-cgi thanks for the PR.

A review with Opus 4.8 says:

I

---

Overall: Well-structured, focused change that solves a real developer pain point (re-running smoke/extended E2E tests without a full redeploy). Good test coverage and docs, plus a couple of nice opportunistic bug fixes. A few items to confirm before approval.

🟢 Done well

• Clean separation of commandHasSkipDeploymentFlag() and getShaFromCommandParts() — correctly supports passing a SHA and the flag in any order on fork PRs.
• Good unit-test coverage for internal true/default-false, fork /test <sha> skip_deployment, and /test-extended.
• Fixes real pre-existing bugs: branchRefid → branchRefId typo, and the debug echo that printed prRefId under the branchRefId label.
• Docs updated in step with the code, including fork examples.

🟠 To address

1. Check status reporting on skip-deployment runs. With skipDeployment == true, deploy_management is skipped, so the entire needs chain feeding e2e_tests_smoke/e2e_tests_custom is skipped. The always() && (inputs.skipDeployment || ...) guards make the tests run, which works — but please confirm the summary job still reports the "Deploy PR / Run E2E Tests (Smoke)" check back to the PR head SHA so the PR status check updates correctly. A short code comment explaining why always() + skipped upstreams produces the intended behavior would help future maintainers.
2. skip_deployment is silently ignored by /test-extended-aad and /test-shared-services. The flag is only wired into /test and /test-extended. A user appending it to the other commands gets a full deploy with no feedback. Consider warning on unsupported use, or confirm this asymmetry is intentional (docs already omit it for those commands).

🟡 Nits

• skip_deployment is a magic string in multiple places — consider a shared constant to avoid drift.
• const { create } = require('domain'); (build.js) is unused dead code; low-risk cleanup while you're here.
• The string→bool round-trip (== 'true') for the typed workflow input is correct — just flagging it's intentional.

Verdict: LGTM pending confirmation of (1) smoke-test check reporting and (2) the intentional flag asymmetry.

---

Let me know if you agree!

[github-actions]

Unit Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit c3e764d.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

.github/workflows/deploy_tre_reusable.yml:887

• Same as the smoke job: when deployment jobs are skipped because of an earlier failure, this condition can still evaluate true (since needs.*.result may be skipped rather than failure). That risks running long custom E2E tests against an environment that never successfully deployed.

  e2e_tests_custom:
    name: "Run E2E Tests"
    if: ${{ always() && inputs.e2eTestsCustomSelector != '' && (inputs.skipDeployment || (!contains(needs.*.result, 'failure') && !contains(needs.*.result, 'cancelled'))) }}
    runs-on: ubuntu-latest

.github/workflows/deploy_tre_reusable.yml:842

• The current job condition will allow smoke tests to run when upstream deployment jobs were skipped due to a failure (e.g., a deploy job fails, downstream needs become skipped, and this expression doesn’t catch that). This can run E2E tests against a partially/un-deployed environment.

This issue also appears on line 884 of the same file.

  e2e_tests_smoke:
    name: "Run E2E Tests (Smoke)"
    if: ${{ always() && (inputs.skipDeployment || (!contains(needs.*.result, 'failure') && !contains(needs.*.result, 'cancelled'))) }}
    runs-on: ubuntu-latest

.github/scripts/build.test.js:416

• Unit tests cover skip_deployment for maintainer PRs and for /test on external PRs, but there’s no coverage for /test-extended <sha> skip_deployment on external PRs. Since the SHA parsing changed to ignore the skip_deployment token, adding a test for the external /test-extended variant would protect against regressions (including different token ordering).

      describe(`for '/test-extended'`, () => {
        test(`should set command to 'run-tests-extended'`, async () => {
          const context = createCommentContext({
            username: 'admin',
            body: '/test-extended',
          });
          await getCommandFromComment({ core, context, github });
          expect(outputFor(mockCoreSetOutput, 'command')).toBe('run-tests-extended');
        });

        test(`should set skipDeployment to 'true' when skip_deployment is supplied`, async () => {
          const context = createCommentContext({
            username: 'admin',
            body: '/test-extended skip_deployment',
          });
          await getCommandFromComment({ core, context, github });
          expect(outputFor(mockCoreSetOutput, 'command')).toBe('run-tests-extended');
          expect(outputFor(mockCoreSetOutput, 'skipDeployment')).toBe('true');
        });

[Copilot]

Pull request overview

Copilot reviewed 180 out of 184 changed files in this pull request and generated 3 comments.

Files not reviewed (1)

• ui/app/package-lock.json: Generated file

Suppressed comments (2)

api_app/resources/strings.py:265

• This template exposes the underlying Event Grid exception in the HTTP 503 response via .format(e). Azure SDK exceptions may include endpoint or response details that should remain in logs; return a static client-facing message while retaining logger.exception for diagnostics.
  CHANGELOG.md:9
• The PR description says this change is limited to the PR-bot skip_deployment flag, but this line is part of substantial unrelated FastAPI/Pydantic, authentication, backup-deletion, template, and dependency changes. This makes the stated scope and linked issue inaccurate and prevents focused validation; split the unrelated work into its corresponding PRs, or update the title/description and issue linkage to describe the full scope.

* Upgrade FastAPI to 0.139.2, Starlette to 1.3.1, and compatible OpenTelemetry dependencies, with routing updates for newer FastAPI and Pydantic v2 compatibility.

[Copilot]

Pull request overview

Copilot reviewed 180 out of 184 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• ui/app/package-lock.json: Generated file

Suppressed comments (8)

api_app/models/domain/airlock_request.py:108

• Making type optional allows the newly supported legacy records without a type to load, but every status transition publishes an event through event_grid/event_sender.py:17, which unconditionally evaluates airlock_request.type.value. Submitting or cancelling one of these records therefore updates Cosmos and then fails with a 503/AttributeError. Legacy records need a migrated/inferred type, or event-producing operations must reject them before persisting a status change.
  api_app/services/airlock.py:351
• The exception text from the Azure SDK is returned verbatim to the API caller. Such messages can include endpoint, response, and infrastructure details; the detailed exception is already captured by logger.exception. Return the generic Event Grid error here as well.
  api_app/requirements.txt:26
• The PR is described as a narrowly scoped PR-bot skip_deployment change, but this line is part of an unrelated Pydantic v1→v2/FastAPI migration; the diff also contains authentication, backup, Terraform, and template changes. These materially expand the risk and cannot be reviewed against issue #4940's stated acceptance criteria. Split the unrelated work into its own PR(s), or update the PR metadata and linked scope to accurately describe the combined release.
  CHANGELOG.md:23
• The changelog says the certs bundle for this fix is 0.7.11, while the changed manifest publishes 0.7.12 (templates/shared_services/certs/porter.yaml:4). This gives operators the wrong version to deploy.

* Ignore changes to `ip_tags` on public IP resources to unblock deployments where these tags are set by Azure policy. (`core` 0.16.17, `tre-shared-service-certs` 0.7.11) ([#5019](https://github.com/microsoft/AzureTRE/issues/5019))

templates/workspaces/base/template_schema.json:119

• Although this field is marked updateable and the documentation says the choice can be changed before deletion, it only exists under conditional allOf. Patch validation only copies updateable fields from the schema's top-level properties (api_app/db/repositories/resources.py:242-244), so a PATCH containing this flag is rejected. Expose conditional updateable properties to patch validation (or redesign the schema) so users can switch the uninstall behavior.
  templates/workspaces/unrestricted/template_schema.json:104
• Although this field is marked updateable and the documentation says the choice can be changed before deletion, it only exists under conditional allOf. Patch validation only copies updateable fields from the schema's top-level properties (api_app/db/repositories/resources.py:242-244), so a PATCH containing this flag is rejected. Expose conditional updateable properties to patch validation (or redesign the schema) so users can switch the uninstall behavior.
  templates/workspaces/airlock-import-review/template_schema.json:82
• Although this field is marked updateable and the documentation says the choice can be changed before deletion, it only exists under conditional allOf. Patch validation only copies updateable fields from the schema's top-level properties (api_app/db/repositories/resources.py:242-244), so a PATCH containing this flag is rejected. Expose conditional updateable properties to patch validation (or redesign the schema) so users can switch the uninstall behavior.
  CHANGELOG.md:9
• The significant /test and /test-extended skip_deployment enhancement described by this PR has no entry in the unreleased changelog. Add an issue-linked entry so release notes include the new PR-bot behavior.

* Upgrade FastAPI to 0.139.2, Starlette to 1.3.1, and compatible OpenTelemetry dependencies, with routing updates for newer FastAPI and Pydantic v2 compatibility.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

.github/workflows/deploy_tre_reusable.yml:52

• This user-facing workflow enhancement has no entry under the Unreleased section of CHANGELOG.md. Repository guidance in .github/copilot-instructions.md:181 requires significant changes to be recorded there with an issue or PR reference.

      skipDeployment:
        description: Skip deployment jobs and run only the requested E2E tests against an existing environment
        type: boolean
        default: false
        required: false

[stuart-bass-cgi]

/test-extended

[github-actions]

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/31794968325 (with refid 66bed7f4)

(in response to this comment from stuart-bass-cgi)

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

.github/scripts/build.js:9

• This user-facing PR bot enhancement is missing the required entry under the unreleased ENHANCEMENTS section of CHANGELOG.md. Please add an entry referencing issue #4940 so the change is included in release notes.

const SKIP_DEPLOYMENT_FLAG = "skip_deployment";

[stuart-bass-cgi]

Marcus Robinson (@marrobi) - should all be resolved now

[stuart-bass-cgi]

/test skip-deployment

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/31800083005 (with refid 66bed7f4)

(in response to this comment from stuart-bass-cgi)

[stuart-bass-cgi]

/test-extended skip-deployment

[github-actions]

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/31804416785 (with refid 66bed7f4)

(in response to this comment from stuart-bass-cgi)

[stuart-bass-cgi]

/test-extended-aad skip_deployment

[github-actions]

🤖 pr-bot 🤖

🏃 Running extended AAD tests: https://github.com/microsoft/AzureTRE/actions/runs/31804624444 (with refid 66bed7f4)

(in response to this comment from stuart-bass-cgi)

[stuart-bass-cgi]

Marcus Robinson (@marrobi) - the review feedback should now be addressed in the latest commits.

• Added comments in .github/workflows/deploy_tre_reusable.yml to explain the always() guards: when skipDeployment is true, the deploy jobs in needs are intentionally skipped, but the smoke/custom E2E jobs still evaluate and run against the existing PR environment.
• Confirmed the summary job still reports the Deploy PR / Run E2E Tests (Smoke) check against prHeadSha; it only depends on the E2E jobs, so it still runs after skip-deployment runs even though deploy jobs are skipped.
• skip_deployment is now explicitly rejected for /test-extended-aad, /test-shared-services, and /test-backups with a bot warning rather than being silently ignored.
• Added a shared SKIP_DEPLOYMENT_FLAG constant, removed the unused domain import, and documented the intentional string-to-bool conversion in the workflow.
• Added unit coverage for unsupported skip_deployment usage and for external /test-extended skip_deployment <sha> ordering.

One caveat on live-testing the unsupported-command warning: issue_comment workflows run from the default branch, so the current live PR bot still loads build.js from main, not this PR. I tried /test-extended-aad skip_deployment; it started a full deploy because main does not yet include the new rejection logic. The behavior is covered by the PR unit tests and will apply to live comments once this parser change reaches main.