Ignore .containerignore for git repositories in ADD

[simonbrauner]

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:

Fixes: #6614

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Local `.containerignore` is no longer applied to ADD from git repositories.

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[Honny1]

LGTM, once @TomSweeneyRedHat 's comment is addressed.

[nalind]

The stageExecutor.performCopy() method sets the "Excludes" field in the options and passes the list of sources to the Add() method. Can't it base the "Excludes" value it passes in on whether or not a source value it's also passing in is a git location?

If need be, the sourceIsGit() helper function could be moved from add.go to an internal package so that performCopy() could call it. There's a check early on in define.TempDirForURL() that could probably stand to use it, too.

[simonbrauner]

@nalind thanks for the review

The stageExecutor.performCopy() method sets the "Excludes" field in the options and passes the list of sources to the Add() method. Can't it base the "Excludes" value it passes in on whether or not a source value it's also passing in is a git location?

If need be, the sourceIsGit() helper function could be moved from add.go to an internal package so that performCopy() could call it. There's a check early on in define.TempDirForURL() that could probably stand to use it, too.

Can you elaborate on this? If I understand this correctly, the idea is to set excludes correctly based on what the source is instead of having two - excludes and gitExcludes, and deciding which one is used later. I am fine with moving sourceIsGit elsewhere so that I can use it in the function that calls Add(), but I am not sure how to use sourceIsGit afterwards.

Because with my understanding:

The Add() method goes through all the sources at once, so there could be both git and non-git sources, so I cannot have one excludes.

Unless:

1. I call Add() for each source separately. Which seems like a strange idea, given that Add() is implemented to handle all the sources at once.
2. I call Add() twice; once for git sources and once for non-git sources. Which could be problematic when ordering matters in case there is ADD local git local git /mount-dir.

[nalind]

2. I call `Add()` twice; once for git sources and once for non-git sources. Which could be problematic when ordering matters in case there is `ADD local git local git /mount-dir`.

That's what we more or less already do when heredocs are also in the mix, so that wouldn't be a new problem.

[simonbrauner]

2. I call `Add()` twice; once for git sources and once for non-git sources. Which could be problematic when ordering matters in case there is `ADD local git local git /mount-dir`.

That's what we more or less already do when heredocs are also in the mix, so that wouldn't be a new problem.

@nalind I changed it in a way that git/nonGit is solved in stage_executor.go. Your comment should be addressed. There is the previous version for reference main...simonbrauner:buildah:issue-6614-v2.

[nalind]

@lsm5 I'm looking at https://artifacts.dev.testing-farm.io/f0d946c9-14ad-4e52-a385-49a751a8d7f6/ and can't reproduce that test failure in a Raw Hide VM with the rhcontainerbot/podman-next COPR enabled. Are the audit logs from the test somewhere that I couldn't spot?

[nalind]

@nalind I changed it in a way that git/nonGit is solved in stage_executor.go. Your comment should be addressed. There is the previous version for reference main...simonbrauner:buildah:issue-6614-v2.

Great! LGTM

[lsm5]

@lsm5 I'm looking at https://artifacts.dev.testing-farm.io/f0d946c9-14ad-4e52-a385-49a751a8d7f6/ and can't reproduce that test failure in a Raw Hide VM with the rhcontainerbot/podman-next COPR enabled.

Just to make sure, you did not install buildah from podman-next but only the other deps, correct? podman-next packages have a super-high Epoch so if you didn't --exclude buildah while installing from podman-next, it'll very likely use buildah from podman-next which imo is not the one you want here.

Are the audit logs from the test somewhere that I couldn't spot?

Are you looking for avc denials? I don't think testing-farm/tmt setup captures them by default, but I'll ask around.

[lsm5]

Are the audit logs from the test somewhere that I couldn't spot?

I've filed a support ticket with testing-farm people

[nalind]

@lsm5 I'm looking at https://artifacts.dev.testing-farm.io/f0d946c9-14ad-4e52-a385-49a751a8d7f6/ and can't reproduce that test failure in a Raw Hide VM with the rhcontainerbot/podman-next COPR enabled.

Just to make sure, you did not install buildah from podman-next but only the other deps, correct? podman-next packages have a super-high Epoch so if you didn't --exclude buildah while installing from podman-next, it'll very likely use buildah from podman-next which imo is not the one you want here.

Updated to versions from the COPR, checked out this PR, built it, ran the in-tree versions of the tests. Also symlinked my tree's "bin" directory into /usr/share/buildah/test and ran the copy from /usr/share/buildah/test/system/ provided by the COPR's buildah-tests package. make forces the label type on the built copy to container_runtime_exec_t, which is the same one we apply to the version in /usr/bin.

Are the audit logs from the test somewhere that I couldn't spot?

Are you looking for avc denials? I don't think testing-farm/tmt setup captures them by default, but I'll ask around.

Yes. The tests are run as root, right? I do get the same error if I remove /usr/bin/selinuxenabled (by removing libselinux-utils), which the test uses to determine if $zflag needs to be set to "z" or left empty.

[nalind]

@containers/buildah-maintainers PTAL

[TomSweeneyRedHat]

LGTM

[TomSweeneyRedHat]

/lgtm