-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the same DockerContext and Dockerfile paths as the CDK is using #4074
Merged
hawflau
merged 16 commits into
aws:develop
from
i18n-tribe:use_the_same_docker_context_and_dockerfile_paths_as_the_cdk_is_using
Nov 4, 2022
+458
−14
Merged
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
a8a5775
Use the same DockerContext and Dockerfile paths as the CDK is using
i18n-tribe a142c25
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
hawflau ec8d936
Add tests to ensure `Dockerfile` can be a path to a docker file via s…
i18n-tribe 8cc2b5d
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
moelasmar 178ddb9
Add tests to ensure SAM supports CDK apps where Docker image asset `f…
i18n-tribe 04a53ba
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
i18n-tribe 5fcfdd6
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
moelasmar 1080378
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
moelasmar af2834c
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
moelasmar 06d5f45
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
moelasmar 15d9f4e
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
moelasmar 1dbd4d2
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
moelasmar 447e939
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
mndeveci 7527fd7
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
mndeveci 88092f0
Merge branch 'develop' into use_the_same_docker_context_and_dockerfil…
hawflau b36eaa0
Fixed failing test after merge conflict with commit fa26bff1415485e36…
i18n-tribe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
16 changes: 16 additions & 0 deletions
16
...cdk/testdata/src/docker/ImagesWithSharedCode/DockerImageFunctionWithSharedCode/Dockerfile
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
FROM public.ecr.aws/lambda/nodejs:14 | ||
|
||
# Add the lambda handler for this feature | ||
COPY DockerImageFunctionWithSharedCode/app.js ./ | ||
|
||
# Add the shared code | ||
COPY SharedCode/ ./SharedCode | ||
|
||
# Add the shared dependencies | ||
COPY package.json ./ | ||
|
||
# Install the dependencies | ||
RUN npm install | ||
|
||
# Set the CMD to your handler (could also be done as a parameter override outside of the Dockerfile) | ||
CMD [ "app.lambdaHandler" ] |
24 changes: 24 additions & 0 deletions
24
...ion/cdk/testdata/src/docker/ImagesWithSharedCode/DockerImageFunctionWithSharedCode/app.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
var gen = require('unique-names-generator'); | ||
const {sayHelloWorld} = require("./SharedCode/shared"); | ||
|
||
const colorName = gen.uniqueNamesGenerator({ | ||
dictionaries: [gen.colors] | ||
}); | ||
|
||
|
||
exports.lambdaHandler = async(event, context) => { | ||
let response; | ||
|
||
try { | ||
response = { | ||
'statusCode': 200, | ||
'body': JSON.stringify({ | ||
message: sayHelloWorld("docker image function construct"), | ||
}), | ||
}; | ||
} catch (err) { | ||
console.log(err); | ||
return err; | ||
} | ||
return response; | ||
}; |
16 changes: 16 additions & 0 deletions
16
.../cdk/testdata/src/docker/ImagesWithSharedCode/FunctionImageAssetWithSharedCode/Dockerfile
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
FROM public.ecr.aws/lambda/nodejs:14 | ||
|
||
# Add the lambda handler for this feature | ||
COPY FunctionImageAssetWithSharedCode/app.js ./ | ||
|
||
# Add the shared code | ||
COPY SharedCode/ ./SharedCode | ||
|
||
# Add the shared dependencies | ||
COPY package.json ./ | ||
|
||
# Install the dependencies | ||
RUN npm install | ||
|
||
# Set the CMD to your handler (could also be done as a parameter override outside of the Dockerfile) | ||
CMD [ "app.lambdaHandler" ] |
24 changes: 24 additions & 0 deletions
24
...tion/cdk/testdata/src/docker/ImagesWithSharedCode/FunctionImageAssetWithSharedCode/app.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
var gen = require('unique-names-generator'); | ||
const {sayHelloWorld} = require("./SharedCode/shared"); | ||
|
||
const colorName = gen.uniqueNamesGenerator({ | ||
dictionaries: [gen.colors] | ||
}); | ||
|
||
|
||
exports.lambdaHandler = async(event, context) => { | ||
let response; | ||
|
||
try { | ||
response = { | ||
'statusCode': 200, | ||
'body': JSON.stringify({ | ||
message: sayHelloWorld("function construct with image asset"), | ||
}), | ||
}; | ||
} catch (err) { | ||
console.log(err); | ||
return err; | ||
} | ||
return response; | ||
}; |
4 changes: 4 additions & 0 deletions
4
tests/iac_integration/cdk/testdata/src/docker/ImagesWithSharedCode/SharedCode/shared.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
|
||
exports.sayHelloWorld = (from) => { | ||
return `Hello World from ${from} with a Dockerfile that shares code with another Dockerfile`; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit concerned about this change since this will break the current code behaviour, might need to investigate on the current usage volume to decide if we can change this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good move 👍
As noted, I think it's good to investigate this.
From our own investigation...
I would not describe this as
breaking
any current behavior.It does
change
the current behavior.But a
change
is only abreaking
if it causes something to stop working.So - would this
change
cause any currently working code tobreak
?We do not think it will.
To illustrate...
Say your CDK code looks like this:
This works with the current code.
And this will still work after this PR is merged.
Currently - the SAM CLI only works with CDK code where
file
is just theDockerfile
anddirectory
is the full path to theDockerfile
.In contrast, CDK code like this...
...does not currently work with the SAM CLI. But it will work when this PR is merged.
An important question would be –
Why was the code written like this to begin with?
In the PR where this code was introduced – this does not seem to have been discussed in the comments.
What this code behaviour specified in your internal tools e.g. Jira? If so, what were the reasons given?
The goal of this feature is to be compatible with the CDK.
But currently, the code is not supporting an important capability of the CDK - the ability to control the directory that assets are built from.
Both
cdk deploy
andsam build
work fine with metadata like:So it is not clear to us why the code in the PR was been written to ensure
sam cli
always works with metadata like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how SAM CLI handles it. So it was written to match SAM CLI expectations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfuss apologies for the slow reply, I was on vacation.
So I had a look at the SAM CLI code to see how
DockerContext
andDockerfile
metadata are used by the SAM CLI.I found 4 other usages.
Just one usage is relevant to this PR:
In this code block in
samcli/lib/build/app_builder.py
We read the metadata properties:
Then we make
docker_context
into an absolute path:Then we call
docker build
using the python docker client, passing the properties as arguments:So in summary -
Dockerfile
is passed without modification to_docker_client.api.build
as thedockerfile
argumentDockerContext
is converted to an absolute path, and passed to_docker_client.api.build
as thepath
argumentHere is the documentation for '_docker_client.api.build'
And those arguments are:
In my opinion, the documentation for
path
is a little bit ambiguous.Does this mean that
path
is:I would completely understand if something thinks it means
1.
- this is what I thought at first.But if we look at the documentation for
dockerfile
- it sayspath within the build context to the Dockerfile
Because
dockerfile
can be apath within the build context
- it can be a path (potentially via subfolders) to theDockerfile
.So in fact,
2.
is the correct interpretation.This interpretation is reinforced by the
docker cli
documentation for docker buildUsage is:
Where
PATH
will beDockerContext
in the SAM CLI.And the option
--file
will beDockerfile
in the SAM CLI.In the
docker cli
documentation, they refer to something called thebuild context
.The
build context
is very similar toPATH
(AKADockerContext
).The
build context
is a copy of the folder specified inPATH
(DockerContext
), excluding files specified in.dockerignore
.So
--file
(AKADockerfile
) is a path to a docker file within thebuild context
(usually a relative path is used).There is no requirement for the docker file to be in the root of the
build context
.I.E.
--file
(Dockerfile
) can be a path via subfolders to the docker file.Here's the documentation for
--file
.The documentation has examples where
--file
is a path via subfolders to the docker file like:In the example above,
PATH
is.
- the current directory.And there's a quote in this section of the docs.
The use case
where the same set of files are used for multiple builds
is exactly what this PR is trying to solve.This is the use case described in the PR description.
This use-case is supported by the
AWS CDK
.This use-case is also - in fact - already supported by the
SAM CLI
.The
AWS CDK
docs support this use-case explicitly:The
SAM CLI
developer guide docs do not support this use-case explicitly:The
SAM CLI
developer guide sound like interpretation1.
of the documentation for '_docker_client.api.build'.But in fact, the
SAM CLI
code works perfectly well ifDockerfile
is a path via subfolders to the docker file.And I've personally used this 'undocumented feature' in multiple
SAM
projects I have worked on.In other words -
What seems to have happened is...
SAM
projects,Dockerfile
must be a direct child ofDockerContext
SAM
was integrated withCDK
, somebody wrote code to enforce this limitationThe code to enforce this supposed limitation means
SAM
is not fully compatible withCDK
.So this PR is removing that code, to improve support for the
CDK
.Also - it seems likely the supposed limitation never in fact existed, and it's always been possible in
SAM
forDockerfile
to be a path via subfolders to the docker file.So I've created a PR to update the
SAM CLI
developer guide, so people will be aware of this capability of theSAM CLI
.I've also created a PR to update the documentation for the Docker Python Client to be more clear.
It may be possible that unclear documentation in
docker-py
is the original source of the belief thatSAM CLI
requiresDockerfile
to be a direct child ofDockerContext
.A further note -
Above there is a link do documentation for the
docker cli
.The python docker SDK doesn't use the
docker cli
, it uses thedocker api
.So I checked the docker API documentation for build
It takes an argument:
It also takes a tar archive which contains the
build context
- documented as:I.e.
dockerfile
'is typically in the archive's root' - but it can be a path to a dockerfile in a subfolder.And I checked the source code of the docker python sdk, to see how it's interacting with the docker API.
Here's the code where it creates the
build context tar archive
.This is a copy of the folder specified in
path
(AKADockerContext
), excluding files specified in.dockerignore
.And on this line it passes
Dockerfile
to the docker API asdockerfile
.For reference, here are the 3 other usages of
DockerContext
metadata, which are not relevant to this PR:DockerContext
to be an absolute path if necessaryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @i18n-tribe for your contribution and your very detailed explanation. I think you have a point regarding this change, I think we miss understood the
DockerContext
andDockerFile
properties and we assumed that theDockerFile
should be a file in theDockerContext
directory path. I did some investigations to make sure that this change will not break any customer. I think it will work as expected, and fix this bug.my only request is to add some integration testing to cover this case