Containerfile: Add heredoc support for RUN, COPY and ADD#264
Containerfile: Add heredoc support for RUN, COPY and ADD#264openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
heredoc support for RUN, COPY and ADD#264Conversation
e6538b5 to
6c07261
Compare
heredoc support for RUN, COPY and ADDheredoc support for RUN, COPY and ADD
|
Failing conformance is just timeout, other conformance test for |
dispatchers.go
Outdated
|
|
||
| var files []File | ||
| for _, heredoc := range heredocs { | ||
| command := parseCommandForHeredoc(original) |
There was a problem hiding this comment.
Should we check for if command==""
There was a problem hiding this comment.
No its not needed, empty command is expected and code further honors it, there are test in dispatcher_test.go for both the cases and further test in buildah containers/buildah#5092
Here empty command only means that treat default shell or sh as the default command.
For instance in below code command is parsed as empty which means implementation should use default shell as command.
RUN <<EOF
echo "Hello" >> /hello
echo "World!" >> /hello
EOFHere command is parsed as python3 instead of default shell
RUN python3 <<EOF
with open("/hello", "w") as f:
print("Hello", file=f)
print("Something", file=f)
EOF|
Is the |
Did you mean |
33d18fe to
76b595d
Compare
|
Added integration test for all complex use-cases in the buildah PR as well: containers/buildah#5092 |
|
@nalind PTAL |
nalind
left a comment
There was a problem hiding this comment.
Looks mostly good, would like one more test case covered and some copy/paste errors in tests fixed up.
7858564 to
9bd590b
Compare
|
@nalind PTAL |
dispatchers_test.go
Outdated
|
|
||
| if err := run(&mybuilder, args, nil, flags, original); err != nil { | ||
| if err := run(&mybuilder, args, nil, flags, original, nil); err != nil { | ||
| t.Errorf("dispatchAdd error: %v", err) |
There was a problem hiding this comment.
| t.Errorf("dispatchAdd error: %v", err) | |
| t.Errorf("run error: %v", err) |
There was a problem hiding this comment.
Please do a pass checking that Errorf messages match the function that produced the assorted errors. I'm only easily able to comment on lines that this PR touches.
|
LGTM, with nits about error messages in tests that are mistaken about which function produced the errors that they're complaining about. |
Imagebuilder must honor and support parsing heredoc syntax from `RUN, COPY and ADD` and allow implementers to implement it. Signed-off-by: Aditya R <arajan@redhat.com>
|
@flouthoc: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Woot! Very well done @flouthoc , great work here! |
Imagebuilder must honor and support parsing heredoc syntax from
RUN, COPY and ADDand allow implementers to implement it.Will help in: containers/buildah#3474