fix(ecr-assets): fix repeated deploys of stacks with tar assets #23497
Conversation
|
|
github-actions
bot
added
the
beginning-contributor
github-actions
bot
added
bug
dastbe
force-pushed
the
fix-repeated-tarball-invocations
branch
3 times, most recently
from
1962771 to
419cdbf
Compare
|
ping! |
so unfortunately an empty tarball is not valid from docker's perspective, so won't be loaded and therefore the integration test will fail. here's an example: |
There was a problem hiding this comment.
@dastbe thanks for your patience here. With the test failure using the empty-tarball I think vendoring hello-world is alright. I'd say lets remove the update.sh script cause I don't really see the need to update the image in the tarball for testing purposes and it isn't referenced anywhere in the test.
Additionally investigated changing the test, to either be a unit test or to not actually deploy the stack in the integ test using deploy: { enabled: false } because we don't actually need it to do so, only to build the images with docker. Either way though the build commands don't get run until actual stack deploy so I believe this is in fact the best way to test this for now.
dastbe
force-pushed
the
fix-repeated-tarball-invocations
branch
from
419cdbf to
4342f4b
Compare
mergify
bot
dismissed stale reviews from comcalvi and MrArnoldPalmer
Pull request has been modified.
done
just to clarify you're not asking me to change anything correct? |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
The current implementation fails because it assumes the output of
`docker load` can only be
```
Loaded Image: {image}
```
However, if docker has to take any actions (such as untagging a previous
image) the output will look like
```
{error message}
{error message}
Loaded Image: {image}
```
To fix this, we can simply take the last line of the output via `tail`
before we attempt to extract the image. If the last line isn't of the
correct form, this will fail in an effectively equivalent way to how it
failed previously.
A previous attempt at fixing this (aws#18823) used a more complicated `sed`
command which used a cli flag that is not consistently available. This
approach won't work on all platforms (ex. macos) and is also much less
clear that it's correct.
This change also adds an integration test that tests the tarbell stack, though
it doesn't test the repeated deploy automatically. I have tested that by
running the integration test multiple times with an image modification in
between. We also vendor the tarball of the hello-world docker image, which
results in ~10kb of additional data in the repo. This is about as small as we
can get a vendored image with as straightforward a vendoring process.
fixes aws#18822
dastbe
force-pushed
the
fix-repeated-tarball-invocations
branch
from
4342f4b to
eb45b05
Compare
mergify
bot
dismissed
MrArnoldPalmer’s stale review
Pull request has been modified.
dastbe
requested review from
MrArnoldPalmer
and removed request for
comcalvi
|
When you manually rebase or merge from main, it cancels our reviews, which is why this hasn't merged yet. Please do not take that action in the future, unless using the command |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…23497) The current implementation fails because it assumes the output of `docker load` can only be ``` Loaded Image: {image} ``` However, if docker has to take any actions (such as untagging a previous image) the output will look like ``` {error message} {error message} Loaded Image: {image} ``` To fix this, we can simply take the last line of the output via `tail` before we attempt to extract the image. If the last line isn't of the correct form, this will fail in an effectively equivalent way to how it failed previously. A previous attempt at fixing this (aws#18823) used a more complicated `sed` command which used a cli flag that is not consistently available. This approach won't work on all platforms (ex. macos) and is also much less clear that it's correct. This change also adds an integration test that tests the tarbell stack, though it doesn't test the repeated deploy automatically. I have tested that by running the integration test multiple times with an image modification in between. We also vendor the tarball of the hello-world docker image, which results in ~20kb of additional data in the repo. This is about as small as we can get a vendored image with as straightforward a vendoring process. fixes aws#18822 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The current implementation fails because it assumes the output of
docker loadcan only beHowever, if docker has to take any actions (such as untagging a previous
image) the output will look like
To fix this, we can simply take the last line of the output via
tailbefore we attempt to extract the image. If the last line isn't of the
correct form, this will fail in an effectively equivalent way to how it
failed previously.
A previous attempt at fixing this (#18823) used a more complicated
sedcommand which used a cli flag that is not consistently available. This
approach won't work on all platforms (ex. macos) and is also much less
clear that it's correct.
This change also adds an integration test that tests the tarbell stack, though
it doesn't test the repeated deploy automatically. I have tested that by
running the integration test multiple times with an image modification in
between. We also vendor the tarball of the hello-world docker image, which
results in ~20kb of additional data in the repo. This is about as small as we
can get a vendored image with as straightforward a vendoring process.
fixes #18822
All Submissions:
Adding new Construct Runtime Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license