Skip to content

[DRAFT] feat(AG-437): support ADS releases - #7145

Draft
ifeanyiecheruo wants to merge 2 commits into
mainfrom
feat/ads-release
Draft

[DRAFT] feat(AG-437): support ADS releases#7145
ifeanyiecheruo wants to merge 2 commits into
mainfrom
feat/ads-release

Conversation

@ifeanyiecheruo

@ifeanyiecheruo ifeanyiecheruo commented Aug 17, 2026

Copy link
Copy Markdown

Risk Assessment: Low
API Breaking Change: None

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages
    are release-note ready, emphasizing
    what was changed, not how.
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)

What does this PR do?

The change adds support for releasing the CLI to ADS integration

upload-artifacts.sh: supports an ADS target to upload-artifacts.sh which deploys to the ADS ingestion bucket and honors the expected ADS layout
config.yaml: adds a new release-s3 job that calls upload-artifacts.sh with an ads target. The new release-s3 job uses the team-hammerhead-ads-deploy-tokens context which should provision the following environment variables

ADS_AWS_ACCESS_KEY_ID
ADS_AWS_SECRET_ACCESS_KEY
ADS_AWS_REGION
ADS_S3_BUCKET
ADS_S3_PREFIX

Where should the reviewer start?

config.yaml

How should this be manually tested?

  • build the v2 CLI for windows and linux with fips enabled
  • build the v2 CLI for mac
  • set
    ADS_AWS_ACCESS_KEY_ID
    ADS_AWS_SECRET_ACCESS_KEY
    ADS_AWS_REGION
    ADS_S3_BUCKET
    ADS_S3_PREFIX
  • run bash release-scripts/upload-artifacts.sh ads

What's the product update that needs to be communicated to CLI users?

Internal change only no communication needed

Risk assessment (Low | Medium | High)?

Low

What are the relevant tickets?

AG-437

The change adds support to upload-artifacts.sh for an ADS S3 bucket target
upload-artifacts.sh also honors the expected ADS layout for deployed files

config.yaml adds a new release-s3 job that calls upload-artifacts.sh with an ads target.
The new release-s3 job uses the team-hammerhead-ads-deploy-tokens context which should provision
the following environment variables

ADS_AWS_ACCESS_KEY_ID
ADS_AWS_SECRET_ACCESS_KEY
ADS_AWS_REGION
ADS_S3_BUCKET
ADS_S3_PREFIX
@ifeanyiecheruo
ifeanyiecheruo requested a review from a team as a code owner August 17, 2026 23:01
@ifeanyiecheruo
ifeanyiecheruo marked this pull request as draft August 17, 2026 23:01
@snyk-io

snyk-io Bot commented Aug 17, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Duplicate Job Name 🟠 [major]

The new release-s3 job instance for ADS deployment uses the exact same name ("upload latest/stable") as the existing stable deployment job instance. CircleCI requires job names within a workflow to be unique; this collision will cause a configuration validation error and prevent the pipeline from starting.

name: upload latest/stable
Staging Directory Leak 🟡 [minor]

The upload_ads function creates a temporary directory using mktemp -d but performs cleanup via rm -rf only at the very end of the function. Since the script uses set -e, any failure during the file copy, checksum generation, or S3 upload process will cause the script to exit immediately, leaving the temporary artifacts on the runner's disk. Using a trap would ensure cleanup occurs on both success and failure.

staging_dir=$(mktemp -d)
for mapping in "${AdsFiles[@]}"; do
  source_path=$(trim "${mapping%%=>*}")
  published_name=$(trim "${mapping##*=>}")
  cp "${source_path}" "${staging_dir}/${published_name}"
done

(cd "${staging_dir}" && sha256sum -- * > checksums.txt)

(
  export AWS_ACCESS_KEY_ID="${ADS_AWS_ACCESS_KEY_ID}"
  export AWS_SECRET_ACCESS_KEY="${ADS_AWS_SECRET_ACCESS_KEY}"
  export AWS_DEFAULT_REGION="${ADS_AWS_REGION:-us-east-1}"
  unset AWS_SESSION_TOKEN AWS_PROFILE

  if [ "${DRY_RUN}" == true ]; then
    echo "DRY RUN: uploading to ADS at ${destination}..."
    aws s3 cp "${staging_dir}" "${destination}" --recursive --dryrun
  else
    echo "Uploading to ADS at ${destination}..."
    aws s3 cp "${staging_dir}" "${destination}" --recursive
  fi
)

rm -rf "${staging_dir}"
Redundant Default Value 🟡 [minor]

In upload_ads, ADS_AWS_REGION is explicitly checked as a mandatory variable, and the script exits if it is empty. However, when exporting AWS_DEFAULT_REGION, the code provides a default value of us-east-1. This default is unreachable and logically contradicts the earlier mandatory check.

for required in ADS_AWS_ACCESS_KEY_ID ADS_AWS_SECRET_ACCESS_KEY ADS_AWS_REGION ADS_S3_BUCKET ADS_S3_PREFIX; do
  if [ -z "${!required:-}" ]; then
    echo "Missing required environment variable: ${required}"
    exit 1
  fi
done

destination="s3://${ADS_S3_BUCKET}/${ADS_S3_PREFIX}/${VERSION_TAG}/"

# Copy the ADS files into a staging directory under their published names and
# generate the checksums.txt that accompanies them.
staging_dir=$(mktemp -d)
for mapping in "${AdsFiles[@]}"; do
  source_path=$(trim "${mapping%%=>*}")
  published_name=$(trim "${mapping##*=>}")
  cp "${source_path}" "${staging_dir}/${published_name}"
done

(cd "${staging_dir}" && sha256sum -- * > checksums.txt)

(
  export AWS_ACCESS_KEY_ID="${ADS_AWS_ACCESS_KEY_ID}"
  export AWS_SECRET_ACCESS_KEY="${ADS_AWS_SECRET_ACCESS_KEY}"
  export AWS_DEFAULT_REGION="${ADS_AWS_REGION:-us-east-1}"
📚 Repository Context Analyzed

This review considered 14 relevant code sections from 11 files (average relevance: 0.44)

🤖 Repository instructions applied (from AGENTS.md)

@ifeanyiecheruo ifeanyiecheruo changed the title [DRAFT] feat: Support ADS releases [DRAFT] feat(AG-437): support ADS releases Aug 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor
Fails
🚫

"[DRAFT] feat(AG-437): support ADS releases" is not using a valid commit message format. For commit guidelines, see: CONTRIBUTING.

Warnings
⚠️ There are multiple commits on your branch, please squash them locally before merging!

Generated by 🚫 dangerJS against fd2bab1

@octavian-snyk
octavian-snyk self-requested a review August 19, 2026 12:45
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