Trim StepStates for implicit steps

[imjasonh]

Fixes #338

Proposed Changes

• If a build specifies no source, skip only the first StepState
• If the build specifies source, skip the first two StepStates
• Add tests, and move diffmatchpatch helper to pkg/buildtest

Release Note

.Status.StepStates omits states for implicit steps, so the number of StepStates == number of user-specified build steps

/assign @shashwathi

[knative-prow-robot]

[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:

• OWNERS [ImJasonH]

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

[shashwathi]

Looks like there is no test coverage for this condition skip> len(op.statuses).

[mattmoor]

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.

[imjasonh]

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.

[shashwathi]

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.

I like this idea. We can loop through the Init containers and only surface the errors if there are any. Ignore implicit steps if they are not providing value to the user.

[mattmoor]

/lgtm
/hold

At least one nit, and unsure if you saw the other comments on here.

[knative-metrics-robot]

The following is the coverage report on pkg/.
Say /test pull-knative-build-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/builder/cluster/builder.go	78.4%	81.9%	3.5

[imjasonh]

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.

[mattmoor]

/lgtm
/hold cancel