Skip to content

ci: add gate, preflight and verification to testing image release - #718

Open
nvasiu wants to merge 2 commits into
mainfrom
gate-ecr-release
Open

nvasiu wants to merge 2 commits into
mainfrom
gate-ecr-release

Conversation

@nvasiu

@nvasiu nvasiu commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Issue #, if available:

Related to #716

Description of changes:

The ecr-release.yml action would previously run for every monorepo release, regardless if the release included a new version for the testing package or not. So it was possible for this action to publish the latest changes of the testing package before they were released. This PR updates the action to be safer.

.github/workflows/ecr-release.yml

  • Add a preflight job to parse the release tag, verify the tag matches the source, check if the version already exists in public ECR, and emit a plan.
  • Gates the build on the above preflight checks.
  • Add a post-publish verification that polls public ECR (polls 10 times with 15s wait = 150s total) and confirms that the new version was published.

.github/scripts/parse_testing_version.py

  • Separate script for parsing the testing version from a release tag.

.github/scripts/tests/test_parse_testing_version.py

  • Unit testing for the parsing script.

.github/workflows/test-parser.yml

  • Wire the new script and test into the script-test workflow.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@nvasiu
nvasiu deployed to ai-pr-review-runtime September 10, 2026 23:21 — with GitHub Actions Active
@nvasiu
nvasiu deployed to ai-pr-review-runtime September 10, 2026 23:29 — with GitHub Actions Active
Comment thread .github/workflows/ecr-release.yml Outdated
@github-actions

This comment has been minimized.

@nvasiu nvasiu changed the title ci: gate testing image publish on testing release ci: add gate, preflight and verification to testing image release Sep 11, 2026
@nvasiu
nvasiu deployed to ai-pr-review-runtime September 11, 2026 18:35 — with GitHub Actions Active
Comment thread .github/workflows/ecr-release.yml Outdated
Comment thread .github/workflows/ecr-release.yml Outdated
Comment thread .github/workflows/ecr-release.yml Outdated
Comment thread .github/scripts/parse_testing_version.py Outdated
@github-actions

This comment has been minimized.

@yaythomas yaythomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The preflight and verification confirm that an image tag exists. They do not confirm the image contains what the release names. The Dockerfile installs aws-durable-execution-sdk-python>=1.0.0, resolved at build time and unbounded, so two builds of one testing version can produce different images, and skip-if-exists assumes a version identifies one artifact.

(The other three points are on the relevant lines.)

Comment thread .github/workflows/ecr-release.yml Outdated
Comment thread .github/scripts/parse_testing_version.py Outdated
Comment thread .github/workflows/ecr-release.yml Outdated
@nvasiu
nvasiu deployed to ai-pr-review-runtime September 11, 2026 22:48 — with GitHub Actions Active
Comment thread .github/workflows/ecr-release.yml
Comment thread .github/workflows/ecr-release.yml Outdated
Comment thread .github/workflows/ecr-release.yml
Comment thread .github/workflows/ecr-release.yml Outdated
@github-actions

This comment has been minimized.

@nvasiu
nvasiu deployed to ai-pr-review-runtime September 14, 2026 20:51 — with GitHub Actions Active
Comment thread .github/workflows/ecr-release.yml Outdated
@github-actions

This comment has been minimized.

@nvasiu

nvasiu commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@yaythomas
Re: #718 (review)

Yes, the preflight confirms that a tag exists, but it's not guaranteed that the image matches the release. And skip-if-exists assumes a version maps to one image, which is not true while we have an unbounded SDK dependency.

But this workflow's current policy is that if a version is already published to ECR, we won't ever overwrite it. So it shouldn't ever be a concern that the user will get different SDK versions from the same image version.

But for the sake of reproducing the image on the ECR in the future / visiblity into what SDK version the image is using, we could:

  • Pin the SDK dependency in the Dockerfile.
  • Or label images with which SDK version they are using (doesn't prevent different images per version, but lets us detect them).

I think either of these option would need some more discussion, and are out of scope for this PR. But if we want to implement either of those, they would be compatible with this PR.

@nvasiu
nvasiu force-pushed the gate-ecr-release branch 2 times, most recently from 5d8d07a to a4e387c Compare September 14, 2026 21:38
@nvasiu
nvasiu deployed to ai-pr-review-runtime September 14, 2026 21:45 — with GitHub Actions Active
@nvasiu
nvasiu deployed to ai-pr-review-runtime September 14, 2026 21:59 — with GitHub Actions Active
Comment on lines +266 to +268
concurrency:
group: ecr-release-latest
cancel-in-progress: false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Codex AI review · Finding arf_v1_hinp33mqfxeksg2dtq6ovnhvhu

[P1] Job-level concurrency is still lossy: GitHub retains only one pending job for this group. If a newer version is pending behind another update and a lower backport reaches the group, the backport cancels the newer job; because the survivor only publishes when its own VERSION is newest, it exits and can leave latest on an older image indefinitely. Make every surviving serialized job derive and publish the maximum registry version, or use a queue/lock that preserves every waiter.

echo "::error::Could not list existing tags in public ECR to decide the latest check."
exit 1
}
newest="$(python .github/scripts/is_newest_testing_version.py --candidate "$VERSION" $existing_tags)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Codex AI review · Finding arf_v1_qharx7yloljtuvkrizlf6gdsf3

[P2] newest is sampled once before a 150-second poll, after the serialized update job has released its lock. If a newer release advances latest during that poll, every retry still requires latest to equal this older version and the correctly published release fails spuriously. Keep verification inside the serialized job, or recompute and verify the highest published version on each attempt.

Comment on lines +27 to +34
- name: Require the ECR upload role secret
env:
ECR_UPLOAD_IAM_ROLE_ARN: ${{ secrets.ECR_UPLOAD_IAM_ROLE_ARN }}
run: |
if [[ -z "$ECR_UPLOAD_IAM_ROLE_ARN" ]]; then
echo "::error::Secret ECR_UPLOAD_IAM_ROLE_ARN is not set. Restore it before releasing."
exit 1
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Codex AI review · Finding arf_v1_gghap6si7h5hcg4o4c65wcpv3h

[P2] This credential check runs before determining whether the release contains a testing component. If the secret is absent, SDK/OTel-only releases fail instead of taking the intended no-op path, coupling unrelated releases to ECR configuration. Parse the tag first and run this check only when steps.tag.outputs.tag_version is non-empty.

@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

Found three release-workflow issues. Most importantly, concurrent testing releases can still leave latest stale. The helper tests do not cover the workflow-level concurrency, verification, or non-testing skip paths.

Reviewed commit 5c613bfa1d2a3b11957748b2998c94a7ab4afbdd. Workflow run

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