Use default behavior for file flag

[liboz]

Our team hit this issue today, but with buildx the file argument needs to be specified from the root directory not just the relative directory from the context (PATH) argument .

See:

This updates the readme to reflect this fact.

[crazy-max]

@liboz

but with buildx the file argument needs to be specified from the root directory not just the relative directory from the context (PATH) argument .

This is the same behavior as docker build. See #189 (review)

[liboz]

@crazy-max You're right and I suppose that was mostly some inexperience on our part. I think updating the documents to reflect the fact that the implicit default path includes the context would be a helpful change for everyone involved though.

[muru]

@liboz and @crazy-max Is the default really {context}/Dockerfile? That's what docker build's behaviour is, but not the docker/build-push-action@v2 action's. If I use context: foo without a file argument, it still looks for the Dockerfile in the current working directory. If I specify both context: foo and file: foo/Dockerfile, OTOH, the action works. Possibly because of this line:

file: core.getInput('file') || 'Dockerfile',

So when specifying the the path context, the file has to be set too. Is that intended? It seems a bit redundant. Maybe that line should be something like (pardon my TypeScript):

file: core.getInput('file') || [core.getInput('context'), "Dockerfile"].filter(Boolean).join("/"),

[liboz]

@muru After doing some more poking around, I think you're right and that suggestion makes sense. @crazy-max What do you think about the proposed change so that the default for file is actually context + Dockerfile?

[crazy-max]

@muru @liboz As I said, this is the expected and same behavior for docker build, docker buildx build and build-push-action:

The docker build command builds Docker images from a Dockerfile and a “context”. A build’s context is the set of files located in the specified PATH or URL. The build process can refer to any of the files in the context. For example, your build can use a COPY instruction to reference a file in the context.

You can test it yourself locally if you want:

Given folder structure and files:

project
│   Dockerfile    
│
└───test
│   │   Dockerfile.test
│   │   app.txt

# ./Dockerfile
FROM alpine
RUN echo "Hello world!"

# ./test/Dockerfile.test
FROM alpine
RUN echo "Hello world!"
COPY app.txt ./

And the following commands and expected result:

$ docker build .
OK

$ docker build ./test
failed to solve with frontend dockerfile.v0: failed to read dockerfile: open /var/lib/docker/tmp/buildkit-mount135048529/Dockerfile: no such file or directory

$ docker build -f Dockerfile.test test
failed to solve with frontend dockerfile.v0: failed to read dockerfile: open /var/lib/docker/tmp/buildkit-mount199981535/Dockerfile.test: no such file or directory

$ docker build -f ./test/Dockerfile.test ./test
OK

$ docker build -f ./test/Dockerfile.test .
failed to compute cache key: "/app.txt" not found: not found

[muru]

@crazy-max that's all fine, I'm not disputing that. What I'm talking of is this case:

project   
└───test
│   │   Dockerfile
│   │   app.txt

Here, docker build test will use test/Dockerfile without having to specify it using -f. That's not the case with the action, however.

[crazy-max]

@muru

Here, docker build test will use test/Dockerfile without having to specify it using -f. That's not the case with the action, however.

Yes if you don't specify a Dockerfile, that's right. So the right decision here would be to not default file with Dockerfile? I'm not against that.

[crazy-max]

@muru After what we have just said we could replace this line:

build-push-action/src/context.ts

Line 60 in 0db984c

file: core.getInput('file') || 'Dockerfile',

With:

 file: core.getInput('file'),

And remove:

build-push-action/action.yml

Line 19 in 0db984c

default: './Dockerfile'

And rely on default behavior. I'm ok with that if you want to make these changes @liboz.

Also linked to #90 #169 #189 #215

[crazy-max]

Also update action.yml. See my comment #244 (comment)

[liboz]

@crazy-max Thanks for the comments and hints. I tried to fix all the tests. When I run it locally I only find/execute 44 tests in both the containerized tests and just running yarn run test directly. However, in the CI, it seems to be executing 45 tests (with arguments that don't seem to be in the test file at all). Do you have any idea where that test might be coming from?

[crazy-max]

@liboz I will take a look on the tests. Keep you in touch.

[crazy-max]

@liboz Can you squash your commits and rebase please?

[liboz]

@crazy-max Done

[crazy-max]

@liboz About the failed test, you forgot to remove this line.

[liboz]

@crazy-max Ah, thanks for pointing that out. It was because I hadn't merged master. Thanks for reviewing 😃

[crazy-max]

PTAL @tonistiigi