-
Notifications
You must be signed in to change notification settings - Fork 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
Allow Dockerfile from outside build-context #886
Allow Dockerfile from outside build-context #886
Conversation
Opening as WIP because tests still have to be updated/added |
d36614a
to
df548a6
Compare
Codecov Report
@@ Coverage Diff @@
## master #886 +/- ##
==========================================
- Coverage 53.2% 52.73% -0.47%
==========================================
Files 258 257 -1
Lines 16338 16409 +71
==========================================
- Hits 8692 8653 -39
- Misses 7081 7185 +104
- Partials 565 571 +6 |
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.
Desgin SGTM
Historically, the Dockerfile had to be insde the build-context, because it was sent as part of the build-context. moby/moby@3f6dc81 added support for passing the Dockerfile through stdin, in which case the contents of the Dockerfile is injected into the build-context. This patch uses the same mechanism for situations where the location of the Dockerfile is passed, and its path is outside of the build-context. Before this change: $ mkdir -p myproject/context myproject/dockerfiles && cd myproject $ echo "hello" > context/hello $ echo -e "FROM busybox\nCOPY /hello /\nRUN cat /hello" > dockerfiles/Dockerfile $ docker build --no-cache -f $PWD/dockerfiles/Dockerfile $PWD/context unable to prepare context: the Dockerfile (/Users/sebastiaan/projects/test/dockerfile-outside/myproject/dockerfiles/Dockerfile) must be within the build context After this change: $ mkdir -p myproject/context myproject/dockerfiles && cd myproject $ echo "hello" > context/hello $ echo -e "FROM busybox\nCOPY /hello /\nRUN cat /hello" > dockerfiles/Dockerfile $ docker build --no-cache -f $PWD/dockerfiles/Dockerfile $PWD/context Sending build context to Docker daemon 2.607kB Step 1/3 : FROM busybox ---> 6ad733544a63 Step 2/3 : COPY /hello / ---> 9a5ae1c7be9e Step 3/3 : RUN cat /hello ---> Running in 20dfef2d180f hello Removing intermediate container 20dfef2d180f ---> ce1748f91bb2 Successfully built ce1748f91bb2 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
df548a6
to
a104852
Compare
Added a unit-test. Was also considering updating the e2e test diff --git a/e2e/image/build_test.go b/e2e/image/build_test.go
index b94566d3..61dee27f 100644
--- a/e2e/image/build_test.go
+++ b/e2e/image/build_test.go
@@ -15,16 +15,21 @@ func TestBuildFromContextDirectoryWithTag(t *testing.T) {
dir := fs.NewDir(t, "test-build-context-dir",
fs.WithFile("run", "echo running", fs.WithMode(0755)),
fs.WithDir("data", fs.WithFile("one", "1111")),
- fs.WithFile("Dockerfile", fmt.Sprintf(`
+ )
+ defer dir.Remove()
+
+ // Dockerfile outside of build-context
+ dockerFile := fs.NewFile(t, "test-build-dockerfile-dir",
+ fs.WithContent(fmt.Sprintf(`
FROM %s
COPY run /usr/bin/run
RUN run
COPY data /data
`, fixtures.AlpineImage)))
- defer dir.Remove()
+ defer dockerFile.Remove()
result := icmd.RunCmd(
- icmd.Command("docker", "build", "-t", "myimage", "."),
+ icmd.Command("docker", "build", "-t", "myimage", "-f", dockerFile.Path(), "."),
withWorkingDir(dir))
result.Assert(t, icmd.Expected{Err: icmd.None}) |
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.
LGTM
I don't think we need another e2e test here, the unit test covers it.
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.
LGTM 🐯
cc @tonistiigi
As evidenced by the test failures we've discovered, we'll need to make some changes that ideally work under both Trusty and Xenial. This means that we need to run the application tests in both environments. This PR adds a $BASE_OS env var that can be either "trusty" or "xenial". It uses this env var for docker builds, and modifies the docker build logic to point to a Dockerfile outside of the build context [1]. We make a new directory in the securedrop/ directory called Dockerfiles that contains the Dockerfiles for both environments. By default, the other Makefile targets will use Trusty. Once trusty is EOL, we can delete the Trusty Dockerfile, though it would be prudent to leave the BASE_OS logic in place for the next major OS transition. [1] docker/cli#886
As evidenced by the test failures we've discovered, we'll need to make some changes that ideally work under both Trusty and Xenial. This means that we need to run the application tests in both environments. This PR adds a $BASE_OS env var that can be either "trusty" or "xenial". It uses this env var for docker builds, and modifies the docker build logic to point to a Dockerfile outside of the build context [1]. We make a new directory in the securedrop/ directory called Dockerfiles that contains the Dockerfiles for both environments. By default, the other Makefile targets will use Trusty. Once trusty is EOL, we can delete the Trusty Dockerfile, though it would be prudent to leave the BASE_OS logic in place for the next major OS transition. [1] docker/cli#886
As evidenced by the test failures we've discovered, we'll need to make some changes that ideally work under both Trusty and Xenial. This means that we need to run the application tests in both environments. This PR adds a $BASE_OS env var that can be either "trusty" or "xenial". It uses this env var for docker builds, and modifies the docker build logic to point to a Dockerfile outside of the build context [1]. We make a new directory in the securedrop/ directory called Dockerfiles that contains the Dockerfiles for both environments. By default, the other Makefile targets will use Trusty. Once trusty is EOL, we can delete the Trusty Dockerfile, though it would be prudent to leave the BASE_OS logic in place for the next major OS transition. [1] docker/cli#886
Historically, the Dockerfile had to be insde the build-context, because it was sent as part of the build-context.
moby/moby@3f6dc81 added support for passing the Dockerfile through stdin, in which case the contents of the Dockerfile is injected into the build-context.
This patch uses the same mechanism for situations where the location of the Dockerfile is passed, and its path is outside of the build-context.
Before this change:
After this change:
- What I did
Added support for building from a Dockerfile that's outside the build-context
- How I did it
See above
- How to verify it
See above
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)