Skip to content

feat(sandbox): FTRS-0101 FTRS-01001 Test sandbox#1073

Closed
gurinder-s-brar wants to merge 1 commit intomainfrom
task/FTRS-0101_sandbox-test
Closed

feat(sandbox): FTRS-0101 FTRS-01001 Test sandbox#1073
gurinder-s-brar wants to merge 1 commit intomainfrom
task/FTRS-0101_sandbox-test

Conversation

@gurinder-s-brar
Copy link
Copy Markdown
Contributor

Description

Context


Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

  • I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

Copilot AI review requested due to automatic review settings March 23, 2026 15:47
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the sandbox deployment process so APIM sandbox deployments can be driven by a specific Git ref (e.g., a tagged commit) rather than requiring the tag to be on main, and it adds a small test harness for the tag-parsing script.

Changes:

  • Adjust the sandbox deployment workflow to consistently use inputs.ref || github.ref and to derive ref_name/ref_type/commit_hash from the checked-out ref.
  • Simplify parse-apim-sandbox-tag.sh by removing the “tag must be on main” verification logic.
  • Update sandbox service documentation and add tests for the tag parsing script.

Reviewed changes

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

Show a summary per file
File Description
services/sandbox-dos-ingest/README.md Updates deployment instructions and fixes the relative link to the sandbox pipeline workflow.
scripts/workflow/tests/test_parse_apim_sandbox_tag.py Adds unit-style tests to validate tag parsing behavior for valid/invalid tags.
scripts/workflow/parse-apim-sandbox-tag.sh Removes git-fetch / “ensure tag is on main” logic; keeps parsing + output emission.
.github/workflows/pipeline-deploy-sandbox.yaml Adds ref-resolution step and plumbs ref through to metadata/deploy jobs.
.github/workflows/build-sandbox-images.yaml Removes main-only guards so APIM auth + image publishing can occur for non-main refs.

Comment on lines 134 to 137
- name: Get APIM Access Token
id: get-token
if: ${{ github.ref == 'refs/heads/main' }}
uses: ./.github/actions/authenticate-apim
with:
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

Removing the if: ${{ github.ref == 'refs/heads/main' }} guards means this reusable workflow will now always authenticate to APIM and push images whenever it is invoked (including from pipeline-deploy-application.yaml on task/** and dependabot/** branches). If the intent is to enable pushes for sandbox tag deployments, consider gating these steps on a tag ref (or an explicit workflow input like publish_enabled) to avoid pushing images from every branch build and reducing risk/cost.

Copilot uses AI. Check for mistakes.
@@ -151,7 +149,6 @@ jobs:
api_name: ${{ env.PROXYGEN_API_NAME }}

- name: "Push image to ECR"
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

With the main-branch guards removed, make publish will run for any caller ref. If this workflow is invoked on non-tag branch pushes (e.g. task/**), this will push a new ECR image for each commit hash and can quickly accumulate images. Consider restricting publishing to tag-triggered runs (or add retention/cleanup logic) so only explicitly tagged refs produce pushed images.

Suggested change
- name: "Push image to ECR"
- name: "Push image to ECR"
if: startsWith(github.ref, 'refs/tags/')

Copilot uses AI. Check for mistakes.
Comment on lines 5 to 8
TAG="${GITHUB_REF_NAME:-}"
WORKSPACE_ROOT="${GITHUB_WORKSPACE:-}" || true

ALLOWED_ENVS=(sandbox internal-dev-sandbox)
ALLOWED_SERVICES=(dos-search dos-ingest)
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

parse-apim-sandbox-tag.sh ignores GITHUB_REF_TYPE even though the composite action requires it. This means a workflow_dispatch run can deploy from a branch whose name happens to match the <environment>-<service>-<version> pattern (since the script only validates the name). Consider explicitly checking GITHUB_REF_TYPE == 'tag' (and skipping otherwise) to ensure this workflow only deploys for real tag refs.

Copilot uses AI. Check for mistakes.
env.update(
{
"GITHUB_OUTPUT": str(github_output),
"GITHUB_REF_NAME": ref_name,
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

The composite action sets GITHUB_REF_TYPE and GITHUB_SHA when running this script, but the test harness only sets GITHUB_REF_NAME. To better mirror the real workflow environment (and catch future regressions if the script starts using these variables), consider setting GITHUB_REF_TYPE and GITHUB_SHA in env.update(...) as well.

Suggested change
"GITHUB_REF_NAME": ref_name,
"GITHUB_REF_NAME": ref_name,
"GITHUB_REF_TYPE": "tag",
"GITHUB_SHA": "0000000000000000000000000000000000000000",

Copilot uses AI. Check for mistakes.
@gurinder-s-brar gurinder-s-brar deleted the task/FTRS-0101_sandbox-test branch April 13, 2026 11:48
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.

2 participants