Do not copy useless files into the image #1444

pini-gh · 2020-06-01T13:28:10Z

Move required files into a local 'app' folder and copy the folder
content into the image.

It does not make sense to have the dockerfiles and docker-compose files copied into the image. It wastes resources and it makes docker-compose up --build to rebuild the image each time a docker-compose file is changed.

sgabe · 2020-06-08T21:00:38Z

Might be best to check the other PRs before merging this one.

buchdag · 2021-09-09T09:30:31Z

@pini-gh I'd like to merge this as it somewhat solves the discussion we had in #1769 but there might unfortunately be a lot of people (and PR) that rely on the current location of the nginx.tmpl file for three containers setups.

Maybe keep the nginx.tmpl file at the root and move everything else to /app ?

thaJeztah · 2021-09-09T09:42:14Z

Dockerfile

@@ -26,7 +26,7 @@ RUN wget https://github.com/jwilder/docker-gen/releases/download/$DOCKER_GEN_VER

 COPY network_internal.conf /etc/nginx/

-COPY . /app/
+COPY app /app/


I would recommend using;

COPY app/. /app/

or

COPY /app/. /app/

The difference is "subtle", and (I think) BuildKit made some changes to make those largely equivalent to what it's using now, but there are some situations where the difference can matter.

Adding the trailing /. means "copy the content of the directory to ", whereas without, it means "Copy to . Depending on if <target> exists or not, and wether or not <target> is a directory, this can cause <source> to either land in (in this case) /app/ or /app/app/. See https://docs.docker.com/engine/reference/commandline/cp/#extended-description.

Using the variant with a trailing /. is therefore the "safer" approach, and slightly clearer on the intent.

I guess we need to make the same changes in Dockerfile.alpine as well? https://github.com/nginx-proxy/nginx-proxy/blob/main/Dockerfile.alpine

(Looking at both files, it's probably possible to combine them into a single Dockerfile as well)

Adding the trailing /. means "copy the content of the directory to ", whereas without, it means "Copy to . Depending on if <target> exists or not, and wether or not <target> is a directory, this can cause <source> to either land in (in this case) /app/ or /app/app/. See https://docs.docker.com/engine/reference/commandline/cp/#extended-description.

This is not true about the Dockerfile COPY command. See [1]:

If <src> is a directory, the entire contents of the directory are copied, including filesystem metadata.
Note
The directory itself is not copied, just its contents.

[1] https://docs.docker.com/engine/reference/builder/#copy

I guess we need to make the same changes in Dockerfile.alpine as well?

Sure.

pini-gh · 2021-09-11T22:14:14Z

@pini-gh I'd like to merge this as it somewhat solves the discussion we had in #1769 but there might unfortunately be a lot of people (and PR) that rely on the current location of the nginx.tmpl file for three containers setups.

Maybe keep the nginx.tmpl file at the root and move everything else to /app ?

I think people updating their instances could read release notes and easily adapt their configuration to this path change. But I understand your concerns.

Your call.

junderw · 2021-09-12T00:42:22Z

I think people updating their instances could read release notes and easily adapt their configuration to this path change. But I understand your concerns.

Your call.

This is a breaking change. Usually projects hold off on breaking changes until some significant new feature that requires a breaking change is done, then they can include a bunch of smaller breaking changes.

This change as-is would only be included in the next major release, and no, this change by itself does not merit a major release imo.

pini-gh · 2021-09-12T09:26:52Z

This change as-is would only be included in the next major release, and no, this change by itself does not merit a major release imo.

So be it.

polarathene

Note that due to changes since the last commit here, dhparam.pem.default no longer exists, there is now a directory dhparam/ that would need to be moved to app/ instead. generate-dhparam.sh also has since been removed.

As for the breaking change concern, I imagine a maintainer can pin an open issue, and include release notes in advance of the future release (with that change) to notify users of the change in advance?

That said, the problem the PR is trying to solve since June 2020 could also just update the .dockerignore in the meantime, less discussion/concerns related to that approach and it'd probably have been merged long ago? (probably still a good idea to eventually group the files intended for the image into a subdir instead of interleaved with other files/dirs at the root though 😅 )

Move required files but 'nginx.tmpl' into a local 'app' folder and copy the folder content into the image. 'nginx.tmpl' should be moved as well, but this is a breaking change for configuration with a separate 'docker-gen' container.

tkw1536 · 2022-03-07T17:52:32Z

.dockerignore

+.gitignore
+*.yml
+Dockerfile*
+LICENSE


Legally speaking, isn't it a license violation to not put the LICENSE file inside the Docker image?

The license says:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

IANAL but I would say the Docker Image qualifies as a copy of the software and should thus contain a copy of the license.

Yes, I think it should be included (I thought I left a comment about that, but must've been another repository 😓)

Should it be included at the root of the image or inside /app ?

edit : not arguing against it but not including the License inside the Docker image does not seem to bother other MIT licensed projects like Traefik or Dokku.

To be honest; it's something that's often forgotten, and not sure if there's a clearly defined location for it, although for regular deb / rpm packages, such files (LICENSE, NOTICE) would be installed in (e.g.) /usr/share/doc/<PACKAGE NAME>/.

In addition, it's also possible to set one or more LABEL to provide some metadata (see https://github.com/opencontainers/image-spec/blob/main/annotations.md#pre-defined-annotation-keys), although this can be somewhat tricky if the image is based on a base image and contains other software (with different licensing), so not sure if adding "partial" information is better than "not including it".

The GitHub Actions workflow uses docker/metadata-action@v3 to tag the image, so we're already setting some org.opencontainers.image labels on the images, including licenses :

org.opencontainers.image.title=nginx-proxy org.opencontainers.image.description=Automated nginx proxy for Docker containers using docker-gen org.opencontainers.image.url=https://github.com/nginx-proxy/nginx-proxy org.opencontainers.image.source=https://github.com/nginx-proxy/nginx-proxy org.opencontainers.image.version=latest org.opencontainers.image.created=2022-03-07T15:23:02.325Z org.opencontainers.image.revision=6b48e11e5cbe8066a619300fa7b40408da02387c org.opencontainers.image.licenses=MIT

I really have no preference over putting the LICENSE file at the root or inside /app.

Ah! Didn't check if it was using the metadata-action, nice!

Yes, no preference either where to stash the file(s) inside the image.

not arguing against it but not including the License inside the Docker image does not seem to bother other MIT licensed projects like Traefik or Dokku.

I can't speak for other projects, but according to the license the full text and copyright notice should be included in the image.
From what I can see the metadata-action only puts in the label 'MIT' and links to the repository - this isn't the same.
My personal preference is to include the LICENSE file directly inside the image.

@tkw1536 @thaJeztah done in 4ea3437

thaJeztah suggested changes Sep 9, 2021

View reviewed changes

pini-gh force-pushed the app-folder branch from f690079 to 6d407e5 Compare September 11, 2021 22:15

pini-gh force-pushed the app-folder branch from 6d407e5 to 8468f9e Compare September 12, 2021 09:25

polarathene reviewed Nov 26, 2021

View reviewed changes

buchdag force-pushed the app-folder branch from 8468f9e to da5e904 Compare March 7, 2022 14:38

pini-gh and others added 2 commits March 7, 2022 16:01

chore: do not copy useless files into the image

5aba125

Move required files but 'nginx.tmpl' into a local 'app' folder and copy the folder content into the image. 'nginx.tmpl' should be moved as well, but this is a breaking change for configuration with a separate 'docker-gen' container.

chore: update .dockerignore

fea6cc7

buchdag force-pushed the app-folder branch from da5e904 to fea6cc7 Compare March 7, 2022 15:03

buchdag merged commit 6b48e11 into nginx-proxy:main Mar 7, 2022

tkw1536 reviewed Mar 7, 2022

View reviewed changes

This was referenced Mar 9, 2022

Include license in Docker image nginx-proxy/acme-companion#930

Merged

Include license in Docker images nginx-proxy/docker-gen#408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not copy useless files into the image #1444

Do not copy useless files into the image #1444

pini-gh commented Jun 1, 2020

sgabe commented Jun 8, 2020

buchdag commented Sep 9, 2021 •

edited

thaJeztah Sep 9, 2021

thaJeztah Sep 9, 2021

pini-gh Sep 11, 2021

pini-gh Sep 11, 2021

pini-gh commented Sep 11, 2021

junderw commented Sep 12, 2021

pini-gh commented Sep 12, 2021

polarathene left a comment •

edited

tkw1536 Mar 7, 2022

thaJeztah Mar 7, 2022

buchdag Mar 8, 2022 •

edited

thaJeztah Mar 8, 2022

buchdag Mar 8, 2022

thaJeztah Mar 9, 2022

tkw1536 Mar 9, 2022

buchdag Mar 9, 2022

Do not copy useless files into the image #1444

Do not copy useless files into the image #1444

Conversation

pini-gh commented Jun 1, 2020

sgabe commented Jun 8, 2020

buchdag commented Sep 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pini-gh commented Sep 11, 2021

junderw commented Sep 12, 2021

pini-gh commented Sep 12, 2021

polarathene left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchdag Mar 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchdag commented Sep 9, 2021 •

edited

polarathene left a comment •

edited

buchdag Mar 8, 2022 •

edited