Allow continuation after comments #29006
Conversation
builder/dockerfile/parser/parser.go
Outdated
There was a problem hiding this comment.
This should be modified to match the "old" matching of comments. Due to the "weird" parsing of empty lines in the old behavior, comments not starting at position 0 were actually seen as empty lines / comments.
stripComments() only matches comments that start at position 0, so something like;
if strings.TrimSpace(newline)[0] == '#' {
continue
}There was a problem hiding this comment.
@tianon gave this short example of how it worked in older versions;
$ docker build -
FROM debian:sid
RUN echo hi \
# wut
&& echo no \
# omg
&& echo yes
Sending build context to Docker daemon 2.048 kB
Step 1 : FROM debian:sid
---> 42e8eb53d800
Step 2 : RUN echo hi && echo no && echo yes
---> Running in 8109ed576ebd
hi
no
yes
---> c661a428bc70
Removing intermediate container 8109ed576ebd
Successfully built c661a428bc70
There was a problem hiding this comment.
Thanks @thaJeztah. The PR has been updated using the test case above. Please take a look and let me know if there are any issues.
This fix is to address moby#29005 where continuation after comments are not allowed after moby#24725. This fix made the change so that moby#29005 could be addressed. An integration test has been added. Other related tests have been udpated to conform to the behavior changes. This fix fixes moby#29005, but please feel free for any additional suggestions. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
e5690e8 to
26aa42e
Compare
vdemeester
left a comment
There was a problem hiding this comment.
LGTM 🐸
/cc @tianon @tiborvass @duglin @vieux
Actually, I think we should only warn ppl about it for now (so reverting the behavior) and in 3 versions, error out.
|
I think this PR is one possible solution, I'll add some points to #29005 so that we can discuss the best option |
|
LGTM |
1 similar comment
|
LGTM |
|
@thaJeztah @vdemeester any blocker on this PR? |
|
This fix is not legit/ proper solution for "Allow continuation after comments".#29006 given by #yongtang |
|
I would tend to prefer reverting it for 1.13 (#29064) and re-adding back with warning and depreciation notice later 👼. |
This fix is to address #29005 where continuation after comments are not allowed after #24725.
This fix made the change so that #29005 could be addressed.
An integration test has been added. Other related tests have been udpated to conform to the behavior changes.
Please feel free for any suggestions.
This fix fixes #29005.
/cc @tianon @duglin @thaJeztah