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

refactor(build): improve err when file specified by -f does not exist #22972

Merged
merged 1 commit into from
Jul 3, 2024
Merged

refactor(build): improve err when file specified by -f does not exist #22972

merged 1 commit into from
Jul 3, 2024
+62 −4

Conversation

BlackHole1
Copy link
Contributor

@BlackHole1 BlackHole1 commented Jun 12, 2024 •
edited
Loading

When the user specifies a Containerfile or Dockfile with the -f flag in podman build, if the file does not exist, the error should be intuitive to the user.

Fixed: #22940

Does this PR introduce a user-facing change?

Improve error message when file specified by -f does not exist in build command

mheon
mheon reviewed Jun 13, 2024
View reviewed changes
cmd/podman/common/build.go Outdated Show resolved Hide resolved
@BlackHole1 BlackHole1 force-pushed the improve-error branch 3 times, most recently from 80d89b0 to f7a3aa2 Compare June 14, 2024 02:56
@BlackHole1
Copy link
Contributor Author

@mheon Modification completed, request re-review

@mheon
Copy link
Member

mheon commented Jun 14, 2024 via email

@baude
Copy link
Member

baude commented Jun 14, 2024

code lgtm, please consider a release note.

@baude
Copy link
Member

baude commented Jun 17, 2024

please rebase

@baude
Copy link
Member

baude commented Jun 17, 2024

do we have a test that uses -f for a non-existent file ?

@BlackHole1
Copy link
Contributor Author

please rebase

Done

do we have a test that uses -f for a non-existent file ?

I checked, and currently, there isn't one. Also, because this logic is within a large function, writing a unit test is quite difficult. Maybe we could add an e2e test?

@BlackHole1 BlackHole1 force-pushed the improve-error branch 2 times, most recently from fb42330 to ee124b0 Compare June 18, 2024 05:40
@rhatdan
Copy link
Member

rhatdan commented Jun 18, 2024

Looks like some tests need to be fixed.

@BlackHole1 BlackHole1 force-pushed the improve-error branch 2 times, most recently from 05397b5 to 5897fdb Compare June 18, 2024 09:57
@BlackHole1
Copy link
Contributor Author

@mheon @baude @rhatdan I added e2e tests and updated the code (which is now a bit more complex than before), please review the PR again.

@mheon
Copy link
Member

mheon commented Jun 18, 2024

LGTM

@mheon
Copy link
Member

mheon commented Jun 18, 2024

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2024
baude
baude reviewed Jun 21, 2024
View reviewed changes
cmd/podman/common/build.go Outdated Show resolved Hide resolved
@baude
Copy link
Member

baude commented Jun 21, 2024

code is fine, i did add a nit comment wondering if we can clean up readability ... will let @Luap99 break the tie.

Luap99
Luap99 reviewed Jun 21, 2024
View reviewed changes
cmd/podman/common/build.go Outdated Show resolved Hide resolved
test/e2e/build_test.go Outdated Show resolved Hide resolved
test/e2e/build_test.go Outdated Show resolved Hide resolved
@packit-as-a-service Packit-as-a-Service
Copy link

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

@BlackHole1
refactor(build): improve err when file specified by -f does not exist
3703cbf 
When the user specifies a Containerfile or Dockfile with the -f flag in podman build, if the file does not exist, the error should be intuitive to the user.

Fixed: containers#22940

Signed-off-by: Kevin Cui <bh@bugs.cc>
@BlackHole1
Copy link
Contributor Author

@Luap99 @baude Done

@BlackHole1
Copy link
Contributor Author

Friendly ping :)

Luap99
Luap99 approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

/lgtm

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

openshift-ci bot commented Jul 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BlackHole1, Luap99, mheon

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-merge-bot openshift-merge-bot bot merged commit b3dab68 into containers:main Jul 3, 2024
86 of 88 checks passed
@BlackHole1 BlackHole1 deleted the improve-error branch July 3, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mheon mheon mheon left review comments

@baude baude baude left review comments

@Luap99 Luap99 Luap99 approved these changes

Assignees

@Luap99 Luap99

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. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Misleading error message when building a nonexistent Containerfile
5 participants
@BlackHole1 @mheon @baude @rhatdan @Luap99