basic support for --keep-git-dir, --unpack, --parents#106
Conversation
|
Dear maintainers, this issue has been impacting EVE-OS project for a while, any chances to get this PR merged? |
c61f8ee to
ad4568b
Compare
|
just rebased with Claude. pinging @m-ildefons |
m-ildefons
left a comment
m-ildefons
left a comment
There was a problem hiding this comment.
Thanks for the PR.
It's just not quite there yet: there are missing test cases and the parser doesn't really model what the syntax reference specifies for the two flags.
| parents :: Parser Parents | ||
| parents = do | ||
| void $ string "--parents" | ||
| return Parents |
There was a problem hiding this comment.
This flag optionally takes an explicit boolean value, i.e. the following three variations are all valid syntax:
COPY --parents foo bar
COPY --parents=true foo bar
COPY --parents=false foo bar
This parser only supports the first case, but would fail to correctly work for the other two cases.
https://docs.docker.com/reference/dockerfile/#copy---parents
| keepGitDir :: Parser KeepGitDir | ||
| keepGitDir = do | ||
| void $ string "--keep-git-dir" | ||
| return KeepGitDir |
There was a problem hiding this comment.
This flag takes an obligatory boolean value, much like the --unpack flag.
This parser doesn't accept that.
| prettyPrintKeepGitDir :: KeepGitDir -> Doc ann | ||
| prettyPrintKeepGitDir keepGitDir = | ||
| case keepGitDir of | ||
| KeepGitDir -> "--keep-git-dir" | ||
| NoKeepGitDir -> mempty |
There was a problem hiding this comment.
This should either produce --keep-git-dir=true, --keep-git-dir=false or nothing, since the boolean value for the flag is not optional.
| data Parents | ||
| = Parents | ||
| | NoParents | ||
| deriving (Show, Eq, Ord) |
There was a problem hiding this comment.
This datastructure doesn't accurately model the --parents flag. It should be more like the data structure Unpack below, because that allows a distinction between the absence of the flag and an explicit --parents=false. The distinction is important to be able to make the pretty printer either omit the flag, or produce an explicit --parents=false.
| it "with keep-git-dir flag" $ | ||
| let file = Text.unlines ["ADD --keep-git-dir foo bar"] | ||
| in assertAst | ||
| file | ||
| [ Add | ||
| ( AddArgs (fmap SourcePath ["foo"]) (TargetPath "bar") ) | ||
| ( AddFlags NoChecksum NoChown NoChmod NoLink KeepGitDir NoUnpack [] ) |
There was a problem hiding this comment.
Missing test cases for --keep-git-dir=true and --keep-git-dir=false
| it "with parents flag" $ | ||
| let file = Text.unlines [ "COPY --parents source /target" ] | ||
| in assertAst | ||
| file | ||
| [ Copy | ||
| ( CopyArgs [ SourcePath "source" ] ( TargetPath "/target" ) ) | ||
| ( CopyFlags NoChown NoChmod NoLink Parents NoSource []) |
There was a problem hiding this comment.
Missing test cases for --parents=true and --parents=false
| it "with just keep-git-dir" $ do | ||
| let add = Add | ||
| ( AddArgs [SourcePath "foo"] (TargetPath "bar") ) | ||
| ( AddFlags NoChecksum NoChown NoChmod NoLink KeepGitDir NoUnpack [] ) | ||
| in assertPretty "ADD --keep-git-dir foo bar" add |
There was a problem hiding this comment.
Missing test cases for --keep-git-dir=true and --keep-git-dir=false
| it "with just parents" $ do | ||
| let copy = Copy | ||
| ( CopyArgs [SourcePath "foo"] (TargetPath "bar") ) | ||
| ( CopyFlags NoChown NoChmod NoLink Parents NoSource [] ) | ||
| in assertPretty "COPY --parents foo bar" copy |
There was a problem hiding this comment.
Missing test cases for --parents=true and --parents=false
Add explicit and implicit versions for --unpack, --parents and --keep-git-dir flags. This means that COPY --unpack foo bar and COPY --unpack=true foo bar are now equivalent and both valid. Similarly for --parents and --keep-git-dir flags as well. Signed-off-by: Moritz Röhrich <moritz@ildefons.de>
|
Again, thanks for the PR. I've gone ahead and fixed it up a bit, so it supports both the explicit and the implicit versions of these flags, i.e. |
3 participants
ADD:COPY: