frontend/dockerfile/dockerignore: assorted cleanups#4062
Merged
jedevc merged 3 commits intomoby:masterfrom Jul 27, 2023
Merged
Conversation
jedevc
reviewed
Jul 25, 2023
thaJeztah
force-pushed
the
cleanup_dockerignore
branch
from
1d86d22 to
971814f
Compare
Member
Could we do the other way around and first have the implementation forked in |
Member
Author
|
My intent was to "mostly" keep the files the same. Then, when we (if we) vendor it from moby/patternmatcher, the PR should just show a "rename" of the files, with no changes (other than package name) |
- don't use a temp-file for the test as all we need is a reader - use a const and string-literal for the test-content, which makes it slightly more readable - don't use hard-coded tests for each line, but use an "expected" slice - don't fail early if line-numbers don't match Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use "doc links" where possible, and better describe the function. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
While this function would usually be used for read a `.dockerignore` file,
it accepts a Reader and can also be used to handle ignore patterns from
other files (e.g. `Dockerfile.dockerignore`) or other sources. The error
was also wrapped multiple times in some code-paths, which could lead to
an error being formatted as:
failed to parse dockerignore: error reading .dockerignore: <some error>
Let's remove mention of the `.dockerignore` filename from the error, and
leave it to the caller to include the filename.
This patch also brings the MainContext dockerignore error inline with the
NamedContext dockerignore error, now printing the exact name of the file.
Co-authored-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
cleanup_dockerignore
branch
from
d0ce775 to
7eb2cea
Compare
Member
Author
|
Updated with @jedevc's patch (I squashed it into the last commit, but added you as co-author; also updated both errors to be "failed parsing ..") |
jedevc
approved these changes
Jul 26, 2023
crazy-max
approved these changes
Jul 27, 2023
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
I'm considering moving this package to https://github.com/moby/patternmatcher (but no final "decision" yet). At least let's clean up this package a bit in case.
frontend/dockerfile/dockerignore: cleanup unit test
slightly more readable
frontend/dockerfile/dockerignore: touch-up godoc and code
Use "doc links" where possible, and better describe the function.
Before:
After:
frontend/dockerfile/dockerignore: remove hard-coded filename from error
While this function would usually be used for read a
.dockerignorefile,it accepts a Reader and can also be used to handle ignore patterns from
other files (e.g.
Dockerfile.dockerignore) or other sources. The errorwas also wrapped multiple times in some code-paths, which could lead to
an error being formatted as:
Let's remove mention of the
.dockerignorefilename from the error, andleave it to the caller to include the filename.