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
No failure reason displayed when build failed using invalid contextDir #13203
No failure reason displayed when build failed using invalid contextDir #13203
Conversation
[test] |
@openshift/devex ptal |
@coreydaley what about s2i flows where the specified context dir doesn't exist? it seems like we could/should be making this check in a more common/shared location, where we are fetching the source. |
@bparees Refactored to do the check in fetchSource, covers both docker and s2i now. |
pkg/build/builder/docker.go
Outdated
d.build.Status.Reason = api.StatusReasonFetchSourceFailed | ||
d.build.Status.Message = api.StatusMessageFetchSourceFailed | ||
switch { | ||
case strings.Contains(err.Error(), "no such file or directory"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you just check if it's an os.IsNotExist error like you did in fetchSource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and better still would probably be to wrap the error inside fetchSource and check for that explicit type. that way if fetchsource throws a file not found error for another reason, you won't report the wrong reason.
@bparees updated with your suggestion |
pkg/build/builder/source.go
Outdated
@@ -92,6 +97,12 @@ func fetchSource(dockerClient DockerClient, dir string, build *api.Build, urlTim | |||
} | |||
} | |||
|
|||
if len(build.Spec.Source.ContextDir) > 0 { | |||
if _, err := os.Stat(filepath.Join(dir, build.Spec.Source.ContextDir)); os.IsNotExist(err) { | |||
return sourceInfo, contextDirNotFoundError(filepath.Join(dir, build.Spec.Source.ContextDir)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just report the contextdir path, "dir" is going to be an internal path with no real meaning to someone who sees it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
lgtm, please create another pull for release-1.5 and also a follow up to add automated tests. |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/767/) (Image: devenv-rhel7_6027) |
Evaluated for origin merge up to db040e1 |
Evaluated for origin test up to db040e1 |
[testextended][extended:core(builds)] |
Evaluated for origin testextended up to db040e1 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/767/) (Base Commit: e019eb7) |
continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/1149/) (Base Commit: e019eb7) (Extended Tests: core(builds)) |
[merge] |
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1419810