Skip to content
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

Surface Step Results in BuildRun Status #871

Merged

Conversation

shahulsonhal
Copy link
Member

@shahulsonhal shahulsonhal commented Sep 21, 2021

Changes

Reference shipwright-io/community#13

Add git commit author result for git source step

Add bundle image digest result for local source code step

Add a function to surface TaksRun results to BuildRun

This PR only includes a subset of the enhancements called for, those are:

and not including the Strategy Results API Proposal that allows strategy authors to surface their desire result.

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

You can now see the TaskRun results in BuildRun status

@openshift-ci openshift-ci bot added the release-note Label for when a PR has specified a release note label Sep 21, 2021
@shahulsonhal
Copy link
Member Author

/kind api-change

@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Sep 21, 2021
@gabemontero
Copy link
Member

hey @shahulsonhal

First, thanks for contributing!

This is an interesting one, both in that I think you are surfacing information users will care about, buin that it surfaces somewhat explicitly TaskRun details in the BuildRun. Admittedly, at least IMO, we have been "walking a tightrope" if you are familiar with the expression, of how much of our Tekton dependency we surface in the API, and how much we do not surface.

I at least need to look at this in more detail, but while the information could be useful, we might need to spend some time on the names of things, minimally. But I should have better input once I can focus on this.

I also think this needs attention from other of our key contributors. I'll assign them as well.

I will also mention we do have an enhancement proposal process (similar to KEPs and TEPs) over at https://github.com/shipwright-io/community ... perhaps we can avoid the need for a SHP for this change, but let's see what kind of consensus we get with the other folks.

/assign @gabemontero
/assign @adambkaplan
/assign @SaschaSchwarze0
/assign @imjasonh
/assign @sbose78

I didn't assign everyone :-) but hopefully that is at least a sufficient meets min (we do have some folks that are out currently)

@gabemontero
Copy link
Member

gabemontero commented Sep 21, 2021

ok @adambkaplan just told me there is a SHP :-) .... apologies

I'll refresh my memory with all that ... and the PR for the SHP was mentioned in the description :-( .... sheesh apologies again

@adambkaplan
Copy link
Member

Copy link
Contributor

@HeavyWombat HeavyWombat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahulsonhal Thanks for PR! This looks pretty solid. So far, I only went through the suggested changes by looking at the code. If time permits, I would like to checkout this branch to test it locally. Until then, maybe you can have a look at my comments already. Almost all of them can be considered nitpicking, but I do think some changes could help with regards to readability or compactness.

docs/buildrun.md Outdated
Comment on lines 236 to 239
- git:
commitAuthor: xxx xxxxxx
commitSha: f25822b85021d02059c9ac8a211ef3804ea8fdde
name: default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Readability.

Suggested change
- git:
commitAuthor: xxx xxxxxx
commitSha: f25822b85021d02059c9ac8a211ef3804ea8fdde
name: default
- name: default
git:
commitAuthor: xxx xxxxxx
commitSha: f25822b85021d02059c9ac8a211ef3804ea8fdde

docs/buildrun.md Outdated
Comment on lines 253 to 255
- bundle:
digest: sha256:0f5e2070b534f9b880ed093a537626e3c7fdd28d5328a8d6df8d29cd3da760c7
name: default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Readability.

Suggested change
- bundle:
digest: sha256:0f5e2070b534f9b880ed093a537626e3c7fdd28d5328a8d6df8d29cd3da760c7
name: default
- name: default
bundle:
digest: sha256:0f5e2070b534f9b880ed093a537626e3c7fdd28d5328a8d6df8d29cd3da760c7

Comment on lines 72 to 76
func PullAndUnpack(
ref name.Reference,
targetPath string,
options ...remote.Option,
) (*v1.Image, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open for debate, but with two arguments and one variadic argument it is still readable in one line.

Suggested change
func PullAndUnpack(
ref name.Reference,
targetPath string,
options ...remote.Option,
) (*v1.Image, error) {
func PullAndUnpack(ref name.Reference, targetPath string, options ...remote.Option) (*v1.Image, error) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion and I will update it.

TBH, I am not a big fan of long lines (it has 104 chars in a single line). It would be better if we consider having a lint rule for the line length.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must mention that I am a big fan of long lines. :-)

Long lines allow me to see more logic because more horizontal lines without scrolling. If details matter, then I can look at the details (for example which variables get passed into a function call) of the longer line.

But just my few cents, within any project with more than one contributor, there'll rarely be a common opinion on those aspects. ;-)

Comment on lines 66 to 72
tr.Status.TaskRunResults = append(tr.Status.TaskRunResults, pipelinev1beta1.TaskRunResult{
Name: "shp-image-digest",
Value: imageDigest,
}, pipelinev1beta1.TaskRunResult{
Name: "shp-image-size",
Value: "230",
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Readability.

Suggested change
tr.Status.TaskRunResults = append(tr.Status.TaskRunResults, pipelinev1beta1.TaskRunResult{
Name: "shp-image-digest",
Value: imageDigest,
}, pipelinev1beta1.TaskRunResult{
Name: "shp-image-size",
Value: "230",
})
tr.Status.TaskRunResults = append(tr.Status.TaskRunResults,
pipelinev1beta1.TaskRunResult{
Name: "shp-image-digest",
Value: imageDigest,
},
pipelinev1beta1.TaskRunResult{
Name: "shp-image-size",
Value: "230",
})

Comment on lines 81 to 83
if err = ioutil.WriteFile(
flagValues.imageDigest, []byte(digest.String()), 0644,
); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Short enough to be on a single line.

Suggested change
if err = ioutil.WriteFile(
flagValues.imageDigest, []byte(digest.String()), 0644,
); err != nil {
if err = ioutil.WriteFile(flagValues.imageDigest, []byte(digest.String()), 0644); err != nil {

@@ -331,4 +331,39 @@ var _ = Describe("Git Resource", func() {
})
})
})

Context("store result", func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Context("store result", func() {
Context("store details in result files", func() {

"fmt"

buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we have no clear line here so far, but new files give us the opportunity to be more specific with the import names. Without an explicit name, the import is just v1beta1 and this is ambiguous.

Suggested change
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
pipeline "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I would not mind to change the build imports to be build instead of buildv1alpha1, but that is just my personal opinion.

Comment on lines 37 to 38
sources.defaultSource.Git = &buildv1alpha1.GitSourceResult{}
sources.bundleSource.Bundle = &buildv1alpha1.BundleSourceResult{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move that into the if blocks that follow? Theoretically speaking, we do not need to initialize both as usually only one of them will be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the initialization to the switch case of func updateBuildRunStatus(), so that it will only initialize the source being used.

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

"github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the other comment, just the "version" is ambiguous.

Suggested change
"github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
build "github.com/shipwright-io/build/pkg/apis/build/v1alpha1"

Comment on lines 40 to 44
"--result-file-image-digest",
fmt.Sprintf(
"$(results.%s-source-%s-bundle-image-digest.path)",
prefixParamsResultsVolumes, name,
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could fit into one line.

Suggested change
"--result-file-image-digest",
fmt.Sprintf(
"$(results.%s-source-%s-bundle-image-digest.path)",
prefixParamsResultsVolumes, name,
),
"--result-file-image-digest", fmt.Sprintf("$(results.%s-source-%s-bundle-image-digest.path)", prefixParamsResultsVolumes, name),

Copy link
Member

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK managed to review the EP before looking at this again :-)

Share similar sentiments with @HeavyWombat on the changes here @shahulsonhal - thanks!

That said, a few minor requests for changes below.

And while it may be obvious to the uninitiated, I'm going to explicitly note (as I did not see an explicit note in the PR description) that this PR takes on a subset of the enhancements called for.

Of course that is fine. Just wanting to make sure it is clear.

In particular, https://github.com/shipwright-io/community/blob/main/ships/0023-surface-results-in-buildruns.md#strategy-results-api-proposal is not tackled with this one.

Also, there was a BR result type noted in the EP that I did not see here. See my question below.

Thanks again.

docs/buildrun.md Outdated
@@ -213,6 +215,46 @@ To make it easier for users to understand why did a BuildRun failed, users can i

In addition, the `Status.Conditions` will host under the `Message` field a compacted message containing the `kubectl` command to trigger, in order to retrieve the logs.

### Step Results in BuildRun Status

After the successful completion of a `BuildRun`, the `.status` field contains the results (`.status.taskResults`) emitted from the `TaskRun` steps. These results contain valuable metadata for users, like the _image digest_ or the _commit sha_ of the source code used for building.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

emitted from the TaskRun steps. -> emitted from the TaskRun steps generate by the BuildRun controller as part of processing the BuildRun.

@@ -277,7 +277,7 @@ Contrary to the strategy `spec.parameters`, you can use system parameters and th

## System results

You can optionally store the size and digest of the image your build strategy created to some files. This information will eventually be made available in the status of the BuildRun.
You can optionally store the size and digest of the image your build strategy created to some files.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some files -> a set of files


// CommitAuthor holds the commit author of a git source
CommitAuthor string `json:"commitAuthor,omitempty"`
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EP calls for a HttpSourceResult as well. Was it an explicit choice to not do that type at this time, and follow up later, or was it an oversight?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was an explicit choice since the http type is an MVP implementation (assuming that everything that comes under .sources is http type) and as of now not emitting any results.

@@ -18,6 +18,7 @@ import (

"github.com/go-git/go-git/v5/plumbing/format/gitignore"
"github.com/google/go-containerregistry/pkg/name"
v1 "github.com/google/go-containerregistry/pkg/v1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the v1 qualifier with the package name ends in v1

surprised govet or goimports did not catch this

either drop the v1 import qualifier, or change to something more meaniful like containerregv1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or containerreg is fine too, as I just noticed the pattern you employed in the new results.go file

@shahulsonhal
Copy link
Member Author

shahulsonhal commented Sep 22, 2021

@gabemontero thanks

And while it may be obvious to the uninitiated, I'm going to explicitly note (as I did not see an explicit note in the PR description) that this PR takes on a subset of the enhancements called for.

I have updated the description.

Also, there was a BR result type noted in the EP that I did not see here. See my question below.

I assume you meant the .status.results field. That is not included in this PR.

@shahulsonhal shahulsonhal force-pushed the enhance-buildrun-status branch 2 times, most recently from 6df9fd2 to 255182f Compare September 23, 2021 15:23
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work. Some suggestions.

cmd/bundle/main.go Outdated Show resolved Hide resolved
cmd/bundle/main.go Outdated Show resolved Hide resolved
docs/buildrun.md Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/results.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/sources/bundle.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/sources/git.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2021
Source related and type related functionalities were not in corresponding files

Add function to append git source results

Add function to append bundle source results

Add function to append output step results

Co-authored-by: Sascha Schwarze <schwarzs@de.ibm.com>
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more items @shahulsonhal.

pkg/apis/build/v1alpha1/buildrun_types.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/sources/bundle.go Outdated Show resolved Hide resolved
commitAuthor := findResultValue(results, fmt.Sprintf("%s-source-%s-%s", prefixParamsResultsVolumes, name, commitAuthorResult))
commitSha := findResultValue(results, fmt.Sprintf("%s-source-%s-%s", prefixParamsResultsVolumes, name, commitSHAResult))

buildRun.Status.Sources = append(buildRun.Status.Sources, buildv1alpha1.SourceResult{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, let's add the if not empty check.

test/e2e/e2e_test.go Outdated Show resolved Hide resolved
@gabemontero
Copy link
Member

I'll add the

/approve

and let @SaschaSchwarze0 and @shahulsonhal sort out the final changes prior to lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 30, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2021
Extend existing test case to add surfacing the taskRun result to buildRun test case

Update output image size type from string to integer
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestions, answering your question regarding the empty check.

pkg/reconciler/buildrun/resources/results.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/results_test.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/sources/bundle.go Outdated Show resolved Hide resolved
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2021
@openshift-merge-robot openshift-merge-robot merged commit c6b6a03 into shipwright-io:main Oct 4, 2021
@adambkaplan adambkaplan added this to the release-v0.6.0 milestone Oct 6, 2021
@SaschaSchwarze0 SaschaSchwarze0 mentioned this pull request Oct 6, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants