-
Notifications
You must be signed in to change notification settings - Fork 159
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ImJasonH 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 |
// is the source-fetching container. | ||
skip++ | ||
} | ||
if skip > len(op.statuses) { |
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.
Looks like there is no test coverage for this condition skip> len(op.statuses)
.
I wonder if we are losing potentially valuable signal when credential initialization fails? Creds has proven to be a source of friction with the Build CRD, and so I just want to make sure we're not making things worse. |
That's a reasonable concern. I think the solution there is to put some more work into surfacing creds-init and source-fetching statuses into the build status itself, probably via termination messages. These implicit steps should be a transparent implementation detail to the user, and they shouldn't need to know how many implicit steps we add at the beginning (and possibly end) of their specified steps, to be able to tell which of their steps may have failed. |
I like this idea. We can loop through the |
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.
/lgtm
/hold
At least one nit, and unsure if you saw the other comments on here.
The following is the coverage report on pkg/.
|
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.
Re: surfacing creds-init and other implicit step failures, we should write those failure messages to statuses and hoist them out into the build's status, rather than having them slip into step states.
I'll work on that in a future PR.
/lgtm |
* checkpoint * Fixed failures, works now * Add test, move diff to buildtest * Update diff.go
* checkpoint * Fixed failures, works now * Add test, move diff to buildtest * Update diff.go
Fixes #338
Proposed Changes
StepState
StepState
sdiffmatchpatch
helper topkg/buildtest
Release Note
/assign @shashwathi