Skip to content
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

qualify docker build context exclusions #248

Closed
wants to merge 1 commit into from

Conversation

qrkourier
Copy link
Member

resolves #247

transplanted this change from #234

@qrkourier qrkourier requested review from a team as code owners February 28, 2024 16:52
@qrkourier qrkourier linked an issue Feb 28, 2024 that may be closed by this pull request
@dovholuknf
Copy link
Member

what's the reason for this and not keeping it broader? i am not seeing the value of this change? is there something this is 'doing' or 'not doing' now?

@qrkourier
Copy link
Member Author

Here's a copy of the explanation from the linked GitHub issue.

The Docker build context unintentionally and incorrectly excludes some common strings at every level, when it's only intending to exclude them at the top level of the context, e.g.,

  • sessions
  • dist

These must be qualified to avoid predictable, difficult-to-diagnose container image build issues, i.e.,

  • /sessions
  • /dist

@dovholuknf
Copy link
Member

did you actually end up having this particular problem then? did you end up actually hitting this issue? I assert that i never want any of these files in the docker container regardless of where they show up in the hierarchy to avoid unintentionally and incorrectly INCLUDING them in the docker build when it's intending to exclude them throughout the docker container.

if you didn't actually have this problem, this seems like a PR that isn't needed. if you DID hit this problem let's explore that.

@qrkourier
Copy link
Member Author

did you actually end up having this particular problem then? did you end up actually hitting this issue? I assert that i never want any of these files in the docker container regardless of where they show up in the hierarchy to avoid unintentionally and incorrectly INCLUDING them in the docker build when it's intending to exclude them throughout the docker container.

if you didn't actually have this problem, this seems like a PR that isn't needed. if you DID hit this problem let's explore that.

The build context does not determine the files in the final image. The build context determines which files are available in the image build environment, and so the files in the final image are always a sub-set of those in the build environment. The unqualified exclusions in the .dockerignore file are too broad and exclude commonly-named files like "sessions" at any level of the source tree.

This change prevents a problem that has not occurred yet and would be subtle to diagnose if it does occur.

The broad category of symptoms I would expect for this problem is that the Docker image build is failing for unclear reasons, or the built image is not functioning correctly.

The cause is that a sub-directory, nested within the source code or build files, named like "sessions" is not available in the image build context, and so the build may fail or a request may get a 404 because an expected file was unintentionally excluded from the build due to excessively-broad exclusions from the build context.

@qrkourier
Copy link
Member Author

@dovholuknf This is ready for review.

1 similar comment
@qrkourier
Copy link
Member Author

@dovholuknf This is ready for review.

@dovholuknf
Copy link
Member

closing. i don't want to make this particular change

@dovholuknf dovholuknf closed this Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

qualify docker build context exclusions
3 participants