-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Update build status to indicate build failure reason #10817
Conversation
@@ -266,6 +266,18 @@ const ( | |||
// secret for pushing the output image. | |||
// The build will stay in the pending state until the secret is created, or the build times out. | |||
StatusReasonMissingPushSecret = "MissingPushSecret" | |||
|
|||
// StatusReasonFailedPostCommitHook indicates that the postcommit failed |
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.
"indicates the post-commit hook failed"
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.
comment shoudl say StatusReasonPostCommitHookFailed
@mfojtik you said these errors should also end up in oc status, right? |
@@ -110,25 +112,39 @@ type detailsStrategy struct { | |||
|
|||
// Prepares a build for update by only allowing an update to build details. | |||
// For now, this is the Spec.Revision field | |||
// We now need to update the build.status.Reason and build.Status.Message |
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.
// build details currently consists of: Spec.Revision, Status.Reason, and Status.Message, all of which are updated from within the build pod
in future yes, that is how I'm thinking about these errors |
oldRevision := oldBuild.Spec.Revision | ||
newRevision := newBuild.Spec.Revision | ||
glog.V(0).Infof("newbuild: %#v", newRevision) | ||
glog.V(0).Infof("oldBuild: %#v", oldRevision) |
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.
extraneous debugging?
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.
yeah. thought i cleaned it up pretty noice, was wrong
b3a5c06
to
eedd08a
Compare
@csrwng thank you for the help, was able to figure it out in the end. |
glog.V(4).Infof("Setting build revision to %#v", d.build.Spec.Revision.Git) | ||
updateBuildRevision(d.build, sourceInfo) | ||
if _, err = d.client.UpdateDetails(d.build); err != nil { | ||
glog.V(0).Infof("error: An error occure while updating the build revision") |
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.
s/occure/occured/
Split the messages out into their own chunk and make a comment that they On Fri, Nov 18, 2016 at 9:00 AM, Ben Parees [email protected]
|
2dee18f
to
4e94444
Compare
No. Create an issue. (but fix your instance of it in this pr of course)
Ben Parees | OpenShift
…On Nov 24, 2016 08:22, "Ionut Palade" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/build/builder/docker.go
<#10817>:
> if sourceInfo != nil {
- updateBuildRevision(d.client, d.build, sourceInfo)
+ glog.V(4).Infof("Setting build revision with details %#v", sourceInfo)
+ revision := updateBuildRevision(d.build, sourceInfo)
+ if updateErr := updateStatusRetrier(d.build, d.client, revision); updateErr != nil {
+ glog.V(0).Infof("error: An error occured while updating the build status: %v", updateErr)
@bparees <https://github.com/bparees> should i create a PR to handle the
errors in the controller and build pods with runtime.HandleError? i see a
lot of them are used without.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10817>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEvl3kT_6yp3DZ3dOWKDWjagIwOHWS5Cks5rBY9_gaJpZM4J121e>
.
|
20dccbb
to
4432145
Compare
[testextended][extended:core(builds|image_ecosystem)] |
@PI-Victor looks like some extended test compilation issues to work through:
|
@bparees yes, seems that KubeRest was removed from there, looking into other test packages to see how to fix this. |
@PI-Victor it is |
4432145
to
6115352
Compare
@mfojtik thanks. |
2ea7681
to
7ab3371
Compare
continuous-integration/openshift-jenkins/test Waiting: Determining build queue position |
7ab3371
to
4b54d01
Compare
Evaluated for origin testextended up to 4b54d01 |
Evaluated for origin test up to 4b54d01 |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11813/) (Base Commit: 44fc84b) |
flake #10663 |
continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/846/) (Base Commit: 74d945c) (Extended Tests: core(builds|image_ecosystem)) |
extended test failure is a known issue. |
@bparees thanks! was hoping this will pass, i've fixed the tests and the one that was faulty. |
yum repo issues. |
flake #10663 |
Evaluated for origin merge up to 4b54d01 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11844/) (Base Commit: 166273b) (Image: devenv-rhel7_5453) |
trello: https://trello.com/c/nAPDRELK/852-5-update-build-status-to-indicate-build-failure-reason-evg