Fix permanent docker asset diff #318
Conversation
| @@ -42,13 +42,17 @@ jobs: | |||
| uses: ./.github/actions/build | |||
|
|
|||
| test: | |||
| needs: | |||
There was a problem hiding this comment.
I noticed that the build/test was failing, but the integration tests were still running. I think it's better to fail fast here.
| SkipRefresh: true, | ||
| Quick: true, | ||
| ExpectRefreshChanges: true, |
There was a problem hiding this comment.
We've fixed most of these issues, but there is still an issue with refresh changes with IAM roles due to #293
| @@ -2,8 +2,7 @@ FROM node:lts AS builder | |||
|
|
|||
| WORKDIR /app | |||
|
|
|||
| COPY package.json yarn.lock ./ | |||
| COPY app ./ | |||
| COPY package.json yarn.lock index.ts ./ | |||
There was a problem hiding this comment.
This way the docker image only changes when the app folder changes not the entire fargate folder.
src/stack.ts
Outdated
| @@ -238,6 +238,7 @@ export class App | |||
| } | |||
|
|
|||
| const app = new cdk.App({ | |||
| outdir: 'cdk.out', | |||
There was a problem hiding this comment.
Alternatively we could keep the default as is and instruct people that use Docker assets to provide this property.
There was a problem hiding this comment.
This file was not formatted.
| @@ -292,7 +296,9 @@ describe('cloud assembly manifest reader', () => { | |||
|
|
|||
| expect(() => { | |||
| AssemblyManifestReader.fromDirectory(path.dirname(manifestFile)); | |||
| }).toThrow(/no such file or directory, open '\/tmp\/foo\/bar\/does\/not\/exist\/MyNestedStack.template.json'/); | |||
| }).toThrow( | |||
| /Failed to read CloudFormation template at path: \/tmp\/foo\/bar\/does\/not\/exist\/MyNestedStack.template.json/, | |||
There was a problem hiding this comment.
We were asserting on the runtime error which must be different between node versions as I was getting a different error locally vs in CI. Updated this to assert on the error that we throw.
README.md
Outdated
| CDK application. | ||
| - `output`: The directory location where the CDK output will be placed. If this | ||
| is not specified it will be placed in a system temporary directory and could | ||
| cause issues with Docker assets. |
There was a problem hiding this comment.
I'd point out what issues exactly, IMO this makes it sound like something will go wrong or the docker images will get mangled.
corymhall
force-pushed
the
corymhall/docker-diff
branch
from
37851dc to
301b3ba
Compare
Docker assets show a permanent diff on preview due to the
cdk.outdirectory using an absolute path to a system temporary directory. Even if the asset does not change, the temporary directory will always be different.There are two ways we could go about solving this:
cdk.Appto always setoutdirto a relativecdk.outdirectoryI opted for the second route mainly because this follows the default behavior of CDK. Almost all CDK applications will have a
cdk.jsonfile to configure some basic settings, one of them being theoutdirsetting. We don't require users to have this file, but we should be strongly encouraging users to have this because there are a couple of additional things that users should set (app, andcontextfeature flags).I've updated the readme to have an additional
Getting Startedsection which matches what we have in https://www.pulumi.com/docs/iac/clouds/aws/guides/cdk/. I've added thecdk.jsonpart (i'll need to also go back and add this to the guide).closes #287