Surface error results to BuildRun #930

dalbar · 2021-11-02T10:37:26Z

Changes

This PR provides a mechanism to surface errors in the build process into BuildRun objects granting more visibility into without being Tekton/Implementation specific. To achieve this goal, we introduce two new generic results shp-error-reason and shp-error-message, that a TaskRun can emit and a BuildRun can pick up.

Initial implementation for ship 24.

/kind feature
/kind api-change

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing (docs will be included once the linked ship progresses)
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

Release Note

Added a new field `FailureDetails` to BuildRun's `Status` that has the failure location of a failed build pod and container. Additionally the new field contains a `Reason` and `Message` to communicate error information to users and third parties.

dalbar · 2021-11-02T10:39:08Z

Continuation of #901.

As requested by @adambkaplan, I have split the error surfacing logic from the git error reporting.

dalbar · 2021-11-02T10:58:42Z

recreating the reviewer list from previous PR:

/unassign @qu1queee
/unassign @shahulsonhal
/assign @SaschaSchwarze0
/assign @adambkaplan
/assign @coreydaley
/assign @otaviof

SaschaSchwarze0

Nice work. Please also extend the buildstrategy.md. Give some guidance on how the two results can be filled - and important: to fail the buildrun, the step still must return a non-zero exit code. We can add examples in later PRs into our sample build strategies.

SaschaSchwarze0

/approve

openshift-ci · 2021-11-03T11:04:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

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:

~~OWNERS~~ [SaschaSchwarze0]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

docs/buildrun.md

docs/buildstrategies.md

gabemontero · 2021-11-05T12:33:57Z

my 2 comments / suggestions have been addressed - thanks @dalbar

@otaviof @adambkaplan @coreydaley - as being assigned to this PR, do you all want to take a review at this before it gets the lgtm tag? If so, please do so. If not, unassign yourselves and we'll from there (I might be able to take a more detailed pass before my EOB today).

thanks

adambkaplan

Most of my comments here are stylistic recommendations. Since build results are effectively "user generated," I recommend that we establish a convention for the provided reason in our documentation and code.

docs/buildrun.md

docs/buildstrategies.md

pkg/reconciler/buildrun/resources/failureDetails_test.go

adambkaplan · 2021-11-12T16:01:45Z

pkg/reconciler/buildrun/resources/failureDetails.go

+const (
+	resultErrorMessage         = "error-message"
+	resultErrorReason          = "error-reason"
+	prefixedResultErrorReason  = prefixParamsResultsVolumes + "-" + resultErrorReason
+	prefixedResultErrorMessage = prefixParamsResultsVolumes + "-" + resultErrorMessage
+)


These constants should be moved to pkg/apis/build/v1alpha1

What is your reasoning for that? It is currently only used in the resources package.

+1 to Baran's assessment. The constants are an implementation detail because we use them on the TaskRun internally but in the BuildRun they are copied into "normal" fields in the status. -> Somebody who uses our Go types does not need to know these constants.

pkg/reconciler/buildrun/resources/failureDetails_test.go

pkg/reconciler/buildrun/resources/failureDetails.go

pkg/reconciler/buildrun/resources/conditions.go

adambkaplan

@dalbar generally looks good - please squash your commits and then I think we can lgtm this.

dalbar · 2021-12-13T19:45:55Z

@dalbar generally looks good - please squash your commits and then I think we can lgtm this.

Thanks. I have squashed the commits.

HeavyWombat · 2021-12-13T20:21:34Z

@dalbar Have you seen these verify remarks:

The pkg/client, pkg/apis package or CRDs contains changes:
deploy/crds/shipwright.io_buildruns.yaml

make: *** [Makefile:115: verify-generate] Error 1
Run make generate to those commit changes!

Can you run a make generate locally?

dalbar · 2021-12-13T21:06:17Z

@dalbar Have you seen these verify remarks:

The pkg/client, pkg/apis package or CRDs contains changes:
deploy/crds/shipwright.io_buildruns.yaml

make: *** [Makefile:115: verify-generate] Error 1
Run make generate to those commit changes!

Can you run a make generate locally?

I did and the command did not yield any changes.

shahulsonhal · 2021-12-14T04:47:28Z

@dalbar Have you seen these verify remarks:
The pkg/client, pkg/apis package or CRDs contains changes:
deploy/crds/shipwright.io_buildruns.yaml

make: *** [Makefile:115: verify-generate] Error 1
Run make generate to those commit changes!
Can you run a make generate locally?
I did and the command did not yield any changes.

@dalbar, Please try the following steps:

Rebase with the main branch
Run make generate (may fail in the first run and ask you to update controller-gen version)
Update the controller gen version (go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.6.2)
Run make generate again

According to ship 0024, this commit adds a mechanism to surface errors from TaskRun steps. The errors are published under `status.failure` in BuildRun iff underlying TaskRun has failed.

SaschaSchwarze0 · 2022-01-03T13:19:47Z

The remaining TODOs (squashing commits and running make generate again) had been performed. Adding lgtm

/lgtm

openshift-ci bot added kind/feature Categorizes issue or PR as related to a new feature. release-note Label for when a PR has specified a release note kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Nov 2, 2021

openshift-ci bot requested review from qu1queee and shahulsonhal November 2, 2021 10:37

dalbar force-pushed the add/surface_error_results branch from 5a6317c to 07f0af5 Compare November 2, 2021 10:50

openshift-ci bot assigned adambkaplan, coreydaley, otaviof and SaschaSchwarze0 Nov 2, 2021

SaschaSchwarze0 requested changes Nov 3, 2021

View reviewed changes

dalbar force-pushed the add/surface_error_results branch 2 times, most recently from b9475f3 to 98399e3 Compare November 3, 2021 10:19

dalbar requested a review from SaschaSchwarze0 November 3, 2021 10:20

SaschaSchwarze0 approved these changes Nov 3, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2021

gabemontero reviewed Nov 3, 2021

View reviewed changes

docs/buildrun.md Outdated Show resolved Hide resolved

gabemontero reviewed Nov 3, 2021

View reviewed changes

docs/buildstrategies.md Outdated Show resolved Hide resolved

dalbar force-pushed the add/surface_error_results branch 2 times, most recently from c743d21 to 51a348e Compare November 3, 2021 20:43

dalbar requested a review from gabemontero November 3, 2021 20:44

coreydaley removed their assignment Nov 5, 2021

adambkaplan requested changes Nov 12, 2021

View reviewed changes

dalbar force-pushed the add/surface_error_results branch 3 times, most recently from d67c464 to f5c483f Compare November 13, 2021 17:17

dalbar force-pushed the add/surface_error_results branch from f5c483f to 595e7d0 Compare November 13, 2021 17:28

HeavyWombat requested a review from adambkaplan November 15, 2021 10:03

dalbar force-pushed the add/surface_error_results branch from 595e7d0 to 0add676 Compare November 15, 2021 11:18

dalbar force-pushed the add/surface_error_results branch from 0add676 to da542ee Compare November 23, 2021 22:38

dalbar force-pushed the add/surface_error_results branch 4 times, most recently from 4d9505d to 495654b Compare December 7, 2021 09:03

HeavyWombat self-requested a review December 10, 2021 08:39

HeavyWombat mentioned this pull request Dec 13, 2021

December 13, 2021 Community Meeting shipwright-io/community#46

Closed

adambkaplan reviewed Dec 13, 2021

View reviewed changes

dalbar force-pushed the add/surface_error_results branch from 495654b to 819300f Compare December 13, 2021 19:44

dalbar force-pushed the add/surface_error_results branch 2 times, most recently from d189b90 to 3ef1606 Compare December 13, 2021 20:57

Surface error results from TaskRun to BuildRun

3a2e93d

According to ship 0024, this commit adds a mechanism to surface errors from TaskRun steps. The errors are published under `status.failure` in BuildRun iff underlying TaskRun has failed.

dalbar force-pushed the add/surface_error_results branch from 3ef1606 to 3a2e93d Compare December 14, 2021 06:15

dalbar requested a review from adambkaplan December 14, 2021 08:50

SaschaSchwarze0 added this to the release-v0.8.0 milestone Jan 3, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2022

openshift-merge-robot merged commit e558e14 into shipwright-io:main Jan 3, 2022

dalbar deleted the add/surface_error_results branch February 26, 2022 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Surface error results to BuildRun #930

Surface error results to BuildRun #930

dalbar commented Nov 2, 2021

dalbar commented Nov 2, 2021

dalbar commented Nov 2, 2021

SaschaSchwarze0 left a comment

SaschaSchwarze0 left a comment

openshift-ci bot commented Nov 3, 2021

gabemontero commented Nov 5, 2021

adambkaplan left a comment

adambkaplan Nov 12, 2021

dalbar Nov 13, 2021 •

edited

Loading

SaschaSchwarze0 Nov 22, 2021

adambkaplan left a comment

dalbar commented Dec 13, 2021

HeavyWombat commented Dec 13, 2021

dalbar commented Dec 13, 2021

shahulsonhal commented Dec 14, 2021

SaschaSchwarze0 commented Jan 3, 2022

Surface error results to BuildRun #930

Surface error results to BuildRun #930

Conversation

dalbar commented Nov 2, 2021

Changes

Submitter Checklist

Release Note

dalbar commented Nov 2, 2021

dalbar commented Nov 2, 2021

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 3, 2021

gabemontero commented Nov 5, 2021

adambkaplan left a comment

Choose a reason for hiding this comment

adambkaplan Nov 12, 2021

Choose a reason for hiding this comment

dalbar Nov 13, 2021 • edited Loading

Choose a reason for hiding this comment

SaschaSchwarze0 Nov 22, 2021

Choose a reason for hiding this comment

adambkaplan left a comment

Choose a reason for hiding this comment

dalbar commented Dec 13, 2021

HeavyWombat commented Dec 13, 2021

dalbar commented Dec 13, 2021

shahulsonhal commented Dec 14, 2021

SaschaSchwarze0 commented Jan 3, 2022

dalbar Nov 13, 2021 •

edited

Loading