Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
f48e551 to
44d8ed0
Compare
nvasiu
changed the title
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. |
5c613bf to
edaee67
Compare
This comment has been minimized.
This comment has been minimized.
edaee67 to
0f4faa3
Compare
This comment has been minimized.
This comment has been minimized.
0f4faa3 to
e1cd63f
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" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| # A lookup that fails (rather than returns nothing) must not fall | ||
| # through to an unconditional push: that would let a transient error | ||
| # downgrade latest. Distinguish absent from failed and fail closed. | ||
| if latest_digest="$(aws ecr-public describe-images \ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if: always() && needs.create-ecr-manifest-per-arch.result == 'success' | ||
| # Serialize the latest update so two releases do not race it. | ||
| concurrency: | ||
| group: ecr-release-latest |
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.
e1cd63f to
3f48987
Compare
| # scheme change or a hand-pushed tag). We cannot prove we are newer, | ||
| # so refuse to overwrite latest and leave it for manual review. | ||
| current="$(python .github/scripts/current_latest_version.py $latest_tags)" | ||
| if [[ -z "$current" ]]; then |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_c6j4c4462phnp2lrkscrhlm65q
[P1] This rejects the registry state produced by the workflow being replaced. Previously, the version manifest used x86_64→arm64 order while latest used arm64→x86_64, so their order-sensitive OCI digests differ and the latest digest has no vX.Y.Z tag. The first testing release after merge therefore exits here and reruns remain stuck. Bootstrap by matching legacy child-platform digests and retagging the exact version manifest, or migrate latest before enabling this guard.
| 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] The earlier preflight check is not a publication lock. Concurrent deliveries for the same testing version can both build through mutable per-architecture tags and reach this push, silently replacing an already-published release or creating a mixed manifest. Use run-scoped architecture tags, then acquire a per-version lock and recheck existence before publishing the immutable version tag.
| # A lookup that fails (rather than returns nothing) must not fall | ||
| # through to an unconditional push: that would let a transient error | ||
| # downgrade latest. Distinguish absent from failed and fail closed. | ||
| if latest_digest="$(aws ecr-public describe-images \ |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_otspk2nucjnfic2v5zdpbwiemx
[P1] Serializing jobs does not make this ECR Public read fresh. After v3 updates latest, an older v2 job can observe stale v1 state, overwrite latest, and then pass verification against another stale read. Use a strongly consistent compare-and-swap high-water mark or a single reconciler that cannot publish a version below the recorded maximum.
| concurrency: | ||
| group: ecr-release-latest | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_hinp33mqfxeksg2dtq6ovnhvhu
[P1] GitHub concurrency retains only one pending job; a newer arrival replaces the existing pending updater even with cancel-in-progress: false. If v3 is running, v4 is pending, and v2 arrives, v4 is canceled and v2 subsequently no-ops against v3, leaving latest behind. Use a queue that preserves every update, or make every surviving job reconcile the highest published immutable version.
Codex AI reviewFour P1 issues remain in the ECR release state machine. Parser tests do not cover the affected legacy and concurrent registry transitions. Reviewed commit |
|
@yaythomas What this workflow currently does:
The above Codex comments describe problems that can only happen if multiple releases run at the same time (runs overwriting the version manifest, an older release updating But in practice we release one version at a time, and can cancel concurrent workflows if needed. And this workflow will self correct on the next release if an error does happen. I think we can go ahead with this PR in order to block our SAM issues that are dependent on an emulator release, and leave the full lock solution for later. Thoughts? |
yaythomas
left a comment
yaythomas
left a comment
There was a problem hiding this comment.
Thank you @nvasiu!
I checked the Codex finding about legacy manifest order (line 358) against public ECR. latest and v1.2.1 currently share one digest, so the first release after merge will not get stuck.
On concurrency, agree that a real lock is out of scope. One cheap change would close most of the gap, though (inline on line 267).
One comment on a file outside this PR: could we add a note to RELEASING.md that only a testing-v tag publishes the emulator image? An sdk-v tag with a testing version bump publishes the package to PyPI but builds no image.
| # A release-tag component naming the testing package: testing-v<x.y.z> with an | ||
| # optional pre-release suffix. Anchored and matched against a single comma-split | ||
| # component so prefixes like "mytesting-v1.2.1" are rejected. | ||
| _COMPONENT = re.compile(r"testing-v([0-9]+\.[0-9]+\.[0-9]+[0-9A-Za-z.-]*)\Z") |
There was a problem hiding this comment.
The two scripts accept different tag grammars. parse_testing_version.py accepts testing-v1.2.3.4 and testing-v1.2.3-x86_64. current_latest_version.py rejects both.
- A
v1.2.3.4release would movelatestto a tag thatcurrent_latest_version.pycannot read. Every later release would then refuse to updatelatest. - A
testing-v1.2.3-x86_64tag would publish a version tag with the same name as an existing per-architecture image tag.
Please could both scripts share one regex, and could you add a testing-v1.2.3.4 test case?
| needs: [preflight, create-ecr-manifest-per-arch] | ||
| if: always() && needs.create-ecr-manifest-per-arch.result == 'success' | ||
| # Serialize the latest update so two releases do not race it. | ||
| concurrency: |
There was a problem hiding this comment.
The concurrency group runs one latest update at a time. GitHub still drops a waiting job when another one arrives. Example: v3 is running, v4 is waiting, and a v2 backport arrives. GitHub cancels v4. v2 then does nothing, because latest already points at v3. So latest stays behind v4.
Please could this job point latest at the highest vX.Y.Z tag in ECR, instead of checking whether this run's version is newer? Then any job that runs repairs latest. A short comment here would help too: the group runs updates one at a time but does not keep every waiting job, and ECR reads can be stale.
| exit 0 | ||
| fi | ||
|
|
||
| docker manifest create "$base" "$base:v$VERSION-x86_64" "$base:v$VERSION-arm64" |
There was a problem hiding this comment.
latest is rebuilt from the per-architecture tags, and those tags can be overwritten. Copying the published index would make latest byte-for-byte identical to the version tag. The version lookup through the latest digest depends on that:
docker buildx imagetools create --tag "$base:latest" "$base:v$VERSION"| ECR_REGISTRY: ${{ steps.login-ecr-public.outputs.registry }} | ||
| ECR_REPOSITORY: ${{ env.ecr_repository_name }} | ||
| run: | | ||
| python -m pip install --upgrade packaging |
There was a problem hiding this comment.
Please could packaging be pinned, like hatch==1.16.5 at line 160? These jobs hold ECR push credentials. (Same at line 405.)
| fi | ||
| fi | ||
|
|
||
| if [[ -n "$current" ]] && ! python -c "import sys; from packaging.version import Version; sys.exit(0 if Version('$VERSION') > Version('${current#v}') else 1)"; then |
There was a problem hiding this comment.
Should a pre-release such as 2.1.0rc1 move latest? If not, we could skip the update when Version(VERSION).is_prerelease is true.
| @@ -89,46 +197,252 @@ jobs: | |||
| create-ecr-manifest-per-arch: | |||
There was a problem hiding this comment.
This job runs checkout and setup-python, but its script only uses aws and docker. Can both steps be dropped?
| env: | ||
| RELEASE_TAG: ${{ github.event.release.tag_name }} | ||
| run: | | ||
| tag_version="$(python .github/scripts/parse_testing_version.py)" |
There was a problem hiding this comment.
The preflight job calls python without setup-python. The other jobs pin 3.13. Could we add it here to match?
| from parse_testing_version import parse_testing_version | ||
|
|
||
|
|
||
| def test_parse_testing_version(): |
There was a problem hiding this comment.
Could this use @pytest.mark.parametrize, like test_current_latest_version.py? The _run_standalone runner in that file can also go, since test-parser.yml runs pytest.
This branch was successfully deployed
2 participants
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.