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
This comment has been minimized.
This comment has been minimized.
5c613bf to
edaee67
Compare
This comment has been minimized.
This comment has been minimized.
edaee67 to
0f4faa3
Compare
| docker manifest create "$base:v$VERSION" "$image_x86_64" "$image_arm64" | ||
| docker manifest annotate "$base:v$VERSION" "$image_arm64" --arch arm64 --os linux | ||
| docker manifest annotate "$base:v$VERSION" "$image_x86_64" --arch amd64 --os linux | ||
| docker manifest push "$base:v$VERSION" |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_pvjgcb3ats4msus2hcvg553pvr
[P1] These commands also run when BUILT_THIS_RUN is false, recreating an already-published v$VERSION from mutable architecture tags. A concurrent retry can update those tags with different image digests and fail; the next recovery then silently rewrites the existing release manifest and verification passes against the rewritten digest. Only create v$VERSION after a fresh build. For recovery and latest, copy the exact existing top-level manifest instead, and test that this path never pushes v$VERSION.
| if printf '%s\n' $existing_tags | grep -qx "v$VERSION"; then | ||
| newest="$(python .github/scripts/latest_testing_version.py $existing_tags)" | ||
| break |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_otspk2nucjnfic2v5zdpbwiemx
[P1] Seeing this run's tag does not prove the complete tag list is fresh. After v3 has updated latest, a serialized v2 backport can receive a successful stale response containing v2 but omitting v3, select v2, and downgrade latest; verification only reports failure and never repairs it. Maintain a strongly consistent serialized high-water mark or use a queue/lock that preserves every updater, refuse downgrades, and reconcile before releasing the lock.
| if not tag.startswith("v"): | ||
| continue | ||
| try: | ||
| version = Version(tag[1:]) |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_bn7u2dvrf6j45ju4qaataf7xkx
[P2] Version() accepts abbreviated PEP 440 values such as 1 and 1.2, although this workflow only publishes versions with an X.Y.Z prefix. A stray registry tag such as v999 is therefore selected as newest, causing attempts to use nonexistent v999-x86_64 and v999-arm64 images and blocking releases. Full-match the supported tag grammar before parsing, and add regression cases for v1 and v1.2.
Codex AI reviewFound two high-impact release-state issues and one tag-validation bug. Tests do not cover the ECR recovery and concurrency paths involved. 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.