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 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| 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)" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| - 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 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
5c613bf to
edaee67
Compare
| if ! existing_tags="$(aws ecr-public describe-images \ | ||
| --region "${{ env.aws_region }}" \ | ||
| --repository-name "$repo_name" \ | ||
| --query 'imageDetails[].imageTags[]' --output text)"; then |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_otspk2nucjnfic2v5zdpbwiemx
[P1] This first registry read after pushing v$VERSION is performed only once. A successful but stale response can omit the new tag; the first release then fails, while later releases can repoint latest to an older image. Poll until the just-pushed version is visible, then compute the maximum from that successful snapshot before creating latest.
| continue | ||
| if highest is None or version > highest: | ||
| highest = version | ||
| return f"v{highest}" if highest is not None else "" |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_lyc34vjl7rp77kru74lga6sulz
[P2] Converting Version back to text normalizes the original tag. For the explicitly accepted v2.0.0-beta, this returns v2.0.0b0, so the workflow searches for architecture tags it never published and the release fails. Track and return the original highest tag, and add a -beta regression case.
| digest_for() { | ||
| aws ecr-public describe-images \ | ||
| --region "${{ env.aws_region }}" \ | ||
| --repository-name "$repo_name" \ | ||
| --image-ids imageTag="$1" \ | ||
| --query 'imageDetails[0].imageDigest' --output text 2>/dev/null || true |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_rrlqy3ac6xtucifop56pikyykn
[P2] digest_for converts every lookup failure into an empty value, and the later equality considers two empty values equal. Concurrent transient failures for latest and the newest version can therefore make verification pass without proving either digest exists. Propagate lookup failures and explicitly reject empty or None digests before comparing them.
Codex AI reviewFound three actionable release-path issues, including a race that can 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.