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.

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

Add support for COPY --exclude and ADD --exclude options #5733

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 12, 2024

Fixes: #5678

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

COPY option  and buildah copy now support --exclude options.

cmd/buildah/addcopy.go Outdated Show resolved Hide resolved
docs/buildah-add.1.md Outdated Show resolved Hide resolved
docs/buildah-copy.1.md Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member Author

rhatdan commented Sep 13, 2024

@flouthoc @giuseppe @nalind @mheon PTAL

@rhatdan
Copy link
Member Author

rhatdan commented Sep 13, 2024

@slinkydeveloper PTAL

@rhatdan
Copy link
Member Author

rhatdan commented Sep 16, 2024

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@nalind
Copy link
Member

nalind commented Sep 16, 2024

This needs quite a few more tests to ensure that we're getting this right. Add conformance tests using multiple ways of expressing the sources to be copied (".", "/", "*"), and tests that combine COPY or ADD using --exclude with an ignorefile, including cases where they disagree on whether or not a particular item should or should not be excluded.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 24, 2024

One issue is docker does not support --exclude in conformance-test right now.

@nalind
Copy link
Member

nalind commented Sep 24, 2024

Set dockerUseBuildKit in the test case to have the client set the right flag in its request to the REST API's build endpoint. I'm not 100% sure that excludes are handled correctly between the client and the daemon and between the daemon and buildkit, but then maybe the odd things I'm seeing are somehow specific to my system.

@rhatdan rhatdan force-pushed the VENDOR branch 2 times, most recently from ee82bf0 to 95d1210 Compare September 24, 2024 19:41
@TomSweeneyRedHat
Copy link
Member

LGTM once comments are addressed and tests are hip

@villesau
Copy link

Is there timeline for landing this? 👀

@rhatdan
Copy link
Member Author

rhatdan commented Oct 10, 2024

@villesau I have been working on other tools, have not come back to change the code that @nalind suggests.

If you want to try to change it go for it.

The actual code is ready to go, the tests are just a little broken.

@villesau
Copy link

@rhatdan Are there some instructions how to set this up and run the tests? I might try, but please do not count on it 😅

dockerfileContents: strings.Join([]string{
"# syntax=docker/dockerfile:1.9-labs",
"FROM scratch",
"COPY --exclude='!**/*-c' ./ subdir/",
Copy link
Member

Choose a reason for hiding this comment

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

(Turns out I was mistaken, and quoting does work for the value of the --exclude= argument.)

docs/buildah-add.1.md Outdated Show resolved Hide resolved
docs/buildah-copy.1.md Outdated Show resolved Hide resolved
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Oct 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe, 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

@rhatdan rhatdan added the lgtm label Oct 17, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 5518774 into containers:main Oct 17, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New --exclude directive from Dockerfile
6 participants