Conversation
This comment has been minimized.
This comment has been minimized.
f48e551 to
44d8ed0
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
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.)
44d8ed0 to
8c205dd
Compare
This comment has been minimized.
This comment has been minimized.
8c205dd to
a7d373d
Compare
This comment has been minimized.
This comment has been minimized.
|
Yes, the preflight confirms that a tag exists, but it's not guaranteed that the image matches the release. And 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:
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. |
5d8d07a to
a4e387c
Compare
a4e387c to
5c613bf
Compare
| concurrency: | ||
| group: ecr-release-latest | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
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)" |
There was a problem hiding this comment.
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.
| - 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 |
There was a problem hiding this comment.
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.
Codex AI reviewFound three release-workflow issues. Most importantly, concurrent testing releases can still leave Reviewed commit |
Issue #, if available:
Related to #716
Description of changes:
The
ecr-release.ymlaction 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.github/scripts/parse_testing_version.py.github/scripts/tests/test_parse_testing_version.py.github/workflows/test-parser.ymlBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.