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.

Sign up for GitHub

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

Jump to bottom

Containerfile: Add heredoc support for RUN, COPY and ADD #264

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Containerfile: Add heredoc support for RUN, COPY and ADD #264

merged 1 commit into from
Nov 10, 2023

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Oct 19, 2023 •
edited
Loading

Imagebuilder must honor and support parsing heredoc syntax from RUN, COPY and ADD and allow implementers to implement it.

Will help in: containers/buildah#3474

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2023
@flouthoc flouthoc marked this pull request as draft October 19, 2023 08:58
@openshift-ci openshift-ci bot requested review from nalind and rhatdan October 19, 2023 08:58
@flouthoc flouthoc mentioned this pull request Oct 19, 2023
Merged
rhatdan
rhatdan reviewed Oct 19, 2023
View reviewed changes
@flouthoc flouthoc force-pushed the heredoc branch 3 times, most recently from e6538b5 to 6c07261 Compare October 23, 2023 08:03
@flouthoc flouthoc marked this pull request as ready for review October 23, 2023 08:03
@flouthoc flouthoc changed the title [WIP] Containerfile: Add heredoc support for RUN, COPY and ADD Containerfile: Add heredoc support for RUN, COPY and ADD Oct 23, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2023
@flouthoc
Copy link
Contributor Author

Following PR is ready for review, please refer to dispatchers_test.go and dockerfile/parser/parser_test.go for newly added tests and some trivial tests are not added as some of the code is imported directly from buildkit and they are tested there.

@rhatdan @nalind PTAL

@flouthoc flouthoc requested a review from rhatdan October 23, 2023 09:04
@flouthoc
Copy link
Contributor Author

Failing conformance is just timeout, other conformance test for Go 1.20 passed.

rhatdan
rhatdan reviewed Oct 23, 2023
View reviewed changes
dispatchers.go Outdated
@@ -354,10 +404,26 @@ func run(b *Builder, args []string, attributes map[string]bool, flagArgs []strin
}
}

var files []File
for _, heredoc := range heredocs {
command := parseCommandForHeredoc(original)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check for if command==""

Copy link
Contributor Author

@flouthoc flouthoc Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
EOF

Here 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

nalind
nalind reviewed Oct 23, 2023
View reviewed changes
@nalind
Copy link
Member

nalind commented Oct 23, 2023

Is the imagebuilder CLI going to start quietly ignoring this syntax, or is it going to continue to flag it as an error because it doesn't understand it?

nalind
nalind reviewed Oct 23, 2023
View reviewed changes
nalind
nalind reviewed Oct 23, 2023
View reviewed changes
nalind
nalind reviewed Oct 23, 2023
View reviewed changes
nalind
nalind reviewed Oct 23, 2023
View reviewed changes
@flouthoc
Copy link
Contributor Author

Is the imagebuilder CLI going to start quietly ignoring this syntax, or is it going to continue to flag it as an error because it doesn't understand it?

Did you mean dockerclient , I have added error for dockerclient implementations.

@flouthoc flouthoc requested a review from nalind October 24, 2023 08:03
TomSweeneyRedHat
TomSweeneyRedHat reviewed Oct 24, 2023
View reviewed changes
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 6, 2023
@flouthoc flouthoc force-pushed the heredoc branch 3 times, most recently from 33d18fe to 76b595d Compare November 7, 2023 06:04
@flouthoc flouthoc marked this pull request as ready for review November 7, 2023 06:04
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2023
@flouthoc
Copy link
Contributor Author

flouthoc commented Nov 7, 2023

Added integration test for all complex use-cases in the buildah PR as well: containers/buildah#5092

@flouthoc
Copy link
Contributor Author

flouthoc commented Nov 7, 2023

@nalind PTAL

nalind
nalind requested changes Nov 8, 2023
View reviewed changes
Copy link
Member

@nalind nalind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, would like one more test case covered and some copy/paste errors in tests fixed up.

@flouthoc flouthoc force-pushed the heredoc branch 2 times, most recently from 7858564 to 9bd590b Compare November 9, 2023 06:33
@flouthoc flouthoc requested a review from nalind November 9, 2023 06:48
@flouthoc
Copy link
Contributor Author

flouthoc commented Nov 9, 2023

@nalind PTAL

nalind
nalind reviewed Nov 9, 2023
View reviewed changes
dispatchers_test.go Outdated
@@ -679,7 +803,7 @@ func TestDispatchRunFlags(t *testing.T) {
args := []string{"echo \"stuff\""}
original := "RUN --mount=type=bind,target=/foo echo \"stuff\""

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
t.Errorf("dispatchAdd error: %v", err)
t.Errorf("run error: %v", err)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nalind
Copy link
Member

nalind commented Nov 9, 2023

LGTM, with nits about error messages in tests that are mistaken about which function produced the errors that they're complaining about.

@flouthoc
Containerfile: Add heredoc support
b0d49c4 
Imagebuilder must honor and support parsing heredoc syntax from `RUN,
COPY and ADD` and allow implementers to implement it.

Signed-off-by: Aditya R <[email protected]>
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 10, 2023

@flouthoc: all tests passed!

Full PR test history. Your PR dashboard.

Instructions 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.

@flouthoc
Copy link
Contributor Author

@nalind @rhatdan Done, PTAL again

@flouthoc flouthoc requested review from nalind and rhatdan November 10, 2023 00:30
@rhatdan
Copy link
Contributor

rhatdan commented Nov 10, 2023

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2023
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 10, 2023

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 35a50d5 into openshift:master Nov 10, 2023
@TomSweeneyRedHat
Copy link
Contributor

Woot! Very well done @flouthoc , great work here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp Awaiting requested review from mrunalp

@TomSweeneyRedHat TomSweeneyRedHat Awaiting requested review from TomSweeneyRedHat

@nalind nalind Awaiting requested review from nalind

@rhatdan rhatdan Awaiting requested review from rhatdan

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@flouthoc @nalind @TomSweeneyRedHat @rhatdan