Don't log EOF error when using podman --remote build with an empty context directory. #19419
Conversation
Romain-Geissler-1A
force-pushed
the
remove-EOF-error-empty-context-with-podman-remote
branch
from
d20d210 to
3294fa1
Compare
|
LGTM |
|
/approve |
openshift-ci
bot
added
the
approved
|
Add this patch for testing or something like it. |
Romain-Geissler-1A
force-pushed
the
remove-EOF-error-empty-context-with-podman-remote
branch
2 times, most recently
from
b9711af to
f1af436
Compare
Thanks, I didn't go back to this pull request until today. I applied your patch, and removed the "rand_content" variable which was unused. |
test/system/070-build.bats
Outdated
| @@ -1097,6 +1097,18 @@ EOF | |||
| run_podman rmi -f build_test | |||
| } | |||
|
|
|||
| @test "podman build empty context dir" { | |||
| tmpdir=$PODMAN_TMPDIR/emptydir | |||
There was a problem hiding this comment.
tmpdir is such a dangerous variable name; something like emptydir or contextdir would be more intuitive to a maintainer.
There was a problem hiding this comment.
I agree. However tmpdir is used everywhere in this file. Do you want me to change only this newly added tmpdir or all of them in this file ? I am asking because different maintainers have different rules, for example in my projects usually I require these changes of names to be done separately, while others are ok to have it done in one commit.
There was a problem hiding this comment.
Ugh, you're right, and several of those are mine. So, in the interests of consistency, you're welcome to leave it as tmpdir and I'll do a cleanup PR later. Or you're welcome to improve the code base by setting a better example in your added test (but no, please don't clean up the others, that is far beyond the scope of this PR). Your choice, and either is fine.
There was a problem hiding this comment.
Ok, I have chosen the name contextdir, hope this is ok.
There was a problem hiding this comment.
Ah I jsut saw there is already PODMAN_contextdir (which is the parent folder of contextdir). So this name might be confusing too. Maybe I will change to buildcontextdir instead.
Romain-Geissler-1A
force-pushed
the
remove-EOF-error-empty-context-with-podman-remote
branch
2 times, most recently
from
2e4c25e to
4498e8e
Compare
test/system/070-build.bats
Outdated
| @@ -1097,6 +1097,18 @@ EOF | |||
| run_podman rmi -f build_test | |||
| } | |||
|
|
|||
| @test "podman build empty context dir" { | |||
| buildcontextdir=$PODMAN_contextdir/emptydir | |||
There was a problem hiding this comment.
You seem to have done a global search/replace. There is no such thing as PODMAN_contextdir. This should be PODMAN_TMPDIR, same as line 1103 below.
…ntext directory. Closes containers#15921. Signed-off-by: Romain Geissler <[email protected]>
Romain-Geissler-1A
force-pushed
the
remove-EOF-error-empty-context-with-podman-remote
branch
from
4498e8e to
4ee31dc
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, rhatdan, Romain-Geissler-1A 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 |
|
LGTM and tests are green. @containers/podman-maintainers PTAL. |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Closes #15921.
I am not sure if you want to add a new test for this.
Does this PR introduce a user-facing change?