Don't log EOF error when using podman --remote build with an empty context directory.#19419
Conversation
d20d210 to
3294fa1
Compare
|
LGTM |
|
/approve |
|
Add this patch for testing or something like it. |
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. |
edsantiago
left a comment
There was a problem hiding this comment.
Two non-blockers. Thanks for your PR!
test/system/070-build.bats
Outdated
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.
2e4c25e to
4498e8e
Compare
edsantiago
left a comment
There was a problem hiding this comment.
Global search/replace caused serious breakage
test/system/070-build.bats
Outdated
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 <romain.geissler@amadeus.com>
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 DetailsNeeds 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 |
Closes #15921.
I am not sure if you want to add a new test for this.
Does this PR introduce a user-facing change?