Add Build status fields to TaskRun status #243

vdemeester · 2018-11-19T16:01:17Z

Updates TaskRunStatus to have the BuildStatus fields (non-deprecated ones) and populate them from the scheduled build status.

Simple naive approach. ~~, putting WIP as I need to add/update tests (unit tests 😉).~~

Closes #236

/cc @pivotal-nader-ziada @imjasonh @bobcatfish

Signed-off-by: Vincent Demeester [email protected]

vdemeester · 2018-11-20T09:00:41Z

/test pull-knative-build-pipeline-integration-tests

pkg/apis/pipeline/v1alpha1/task_types.go

pkg/apis/pipeline/v1alpha1/taskrun_types.go

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

vdemeester · 2018-11-20T16:41:17Z

# […]
I1120 15:23:03.717] status:
I1120 15:23:03.717]   conditions:
I1120 15:23:03.718]   - lastTransitionTime: "2018-11-20T15:16:50Z"
I1120 15:23:03.718]     message: All Tasks have completed executing
I1120 15:23:03.718]     reason: Succeeded
I1120 15:23:03.718]     status: "True"
I1120 15:23:03.718]     type: Succeeded
# […]
I1120 15:23:03.758] --- FAIL: TestHelmDeployPipelineRun (329.85s)
I1120 15:23:03.758] 	helm_task_test.go:177: Image to be pusblished: gcr.io/knative-boskos-09/kbuild-pipeline-e2e-img/go-helloworld-image-sueovuam
I1120 15:23:03.758] 	helm_task_test.go:141: Error from pinging service IP 35.239.144.181 : timed out waiting for the condition

Looking at that error (and the object above that log), it feels like it's flakey 🤔

/test pull-knative-build-pipeline-integration-tests

bobcatfish · 2018-11-20T17:07:12Z

pkg/apis/pipeline/v1alpha1/task_types.go

@@ -68,8 +68,7 @@ type TaskSpec struct {

 // TaskStatus defines the observed state of Task
 type TaskStatus struct {
-	// INSERT ADDITIONAL STATUS FIELD - define observed state of cluster
-	// Important: Run "make" to regenerate code after modifying this file
+	// Important: Run "./hack/update-codegen.sh" to regenerate code after modifying this file


🙏 thank you!

p.s. really appreciate your attention to detail cleaning up lots of comments and docs @vdemeester !!! ❤️

bobcatfish · 2018-11-20T17:08:24Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+		state := build.Status.StepStates[i]
+		tr.Status.Steps = append(tr.Status.Steps, v1alpha1.StepState{
+			ContainerState: *state.DeepCopy(),
+			// FIXME(vdemeester) logsURL


if this is something you intend to fix in a further PR can you add a bit more detail about what needs to happen here?

if you plan to fix it in this PR just ignore me :D

Ah right, I should add more context to this… So there is a LogsURL field in the StepState (previously StepRun), which was not populate (and still is not). That's why the FIXME here, to not forget… but really, to not forget we shoul create an issue, not a comment 😝

pkg/reconciler/v1alpha1/taskrun/taskrun.go

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

bobcatfish

I have strong feelings about moving the test logic down a level, into a unit test and out of the reconciler test.

Thanks so much for taking this on!!

vdemeester · 2018-11-20T17:13:46Z

I have strong feelings about moving the test logic down a level, into a unit test and out of the reconciler test.

Yes, definitely agree on that 🙌 This needs refactoring, and I'll do the refactoring in the PR very shortly 😉

vdemeester · 2018-11-21T12:46:57Z

/test pull-knative-build-pipeline-integration-tests

nader-ziada · 2018-11-21T14:57:08Z

everything looking good @vdemeester but I don't know why the helm test in so flakey for you, I didn't have any trouble with it before

/test pull-knative-build-pipeline-integration-tests

vdemeester · 2018-11-21T15:27:26Z

I think there is currently some issue with gcr.io (at least I have some from my network)

nader-ziada · 2018-11-21T21:50:33Z

/test pull-knative-build-pipeline-integration-tests

nader-ziada

All the comments so far have been addressed. Thanks @vdemeester

/approve
/lgtm

knative-prow-robot · 2018-11-22T14:17:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada, vdemeester

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~~ [pivotal-nader-ziada]

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

nader-ziada · 2018-11-22T14:22:43Z

@vdemeester looks like you might have to rebase

vdemeester · 2018-11-22T14:24:49Z

@pivotal-nader-ziada oh boy 😅 Will do :)

Updates TaskRunStatus to have the BuildStatus fields (non-deprecated ones) and populate them from the scheduled build status. Signed-off-by: Vincent Demeester <[email protected]>

Extract updating the TaskRunStatus from reconcile so we can test it in isolation. Signed-off-by: Vincent Demeester <[email protected]>

Signed-off-by: Vincent Demeester <[email protected]>

knative-metrics-robot · 2018-11-22T14:32:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/taskrun/taskrun.go	72.6%	74.4%	1.9

nader-ziada · 2018-11-22T14:56:34Z

Thanks @vdemeester

/lgtm

knative-prow-robot requested review from bobcatfish, imjasonh and nader-ziada November 19, 2018 16:01

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 19, 2018

vdemeester force-pushed the 236-taskrunstatus branch 2 times, most recently from e93fa49 to 1d97125 Compare November 20, 2018 09:56

vdemeester changed the title ~~WIP: Add Build status fields to TaskRun status~~ Add Build status fields to TaskRun status Nov 20, 2018

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2018

imjasonh requested changes Nov 20, 2018

View reviewed changes

vdemeester force-pushed the 236-taskrunstatus branch 4 times, most recently from 4ef652c to 69a62e0 Compare November 20, 2018 15:04

bobcatfish reviewed Nov 20, 2018

View reviewed changes

pkg/reconciler/v1alpha1/taskrun/taskrun.go Show resolved Hide resolved

bobcatfish reviewed Nov 20, 2018

View reviewed changes

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go Show resolved Hide resolved

bobcatfish reviewed Nov 20, 2018

View reviewed changes

bobcatfish mentioned this pull request Nov 20, 2018

Add taskRun status to pipelineRun Status #244

Merged

vdemeester force-pushed the 236-taskrunstatus branch from 69a62e0 to 74ca9f7 Compare November 21, 2018 11:01

imjasonh mentioned this pull request Nov 21, 2018

Break Pipelines' dependency on Build #252

Closed

nader-ziada reviewed Nov 22, 2018

View reviewed changes

knative-prow-robot assigned nader-ziada Nov 22, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2018

vdemeester added 3 commits November 22, 2018 15:25

Add Build status fields to TaskRun status

f7c9467

Updates TaskRunStatus to have the BuildStatus fields (non-deprecated ones) and populate them from the scheduled build status. Signed-off-by: Vincent Demeester <[email protected]>

Refactor reconcilier and test 💃

a60a382

Extract updating the TaskRunStatus from reconcile so we can test it in isolation. Signed-off-by: Vincent Demeester <[email protected]>

Update pipelinerun_test.go to take new TaskRun into account 👼

968036c

Signed-off-by: Vincent Demeester <[email protected]>

vdemeester force-pushed the 236-taskrunstatus branch from 74ca9f7 to 968036c Compare November 22, 2018 14:29

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2018

knative-prow-robot merged commit c42258c into tektoncd:master Nov 22, 2018

vdemeester deleted the 236-taskrunstatus branch November 22, 2018 14:59

mchmarny unassigned nader-ziada Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Build status fields to TaskRun status #243

Add Build status fields to TaskRun status #243

vdemeester commented Nov 19, 2018 •

edited

Loading

vdemeester commented Nov 20, 2018

vdemeester commented Nov 20, 2018

bobcatfish Nov 20, 2018

bobcatfish Nov 20, 2018

bobcatfish Nov 20, 2018

vdemeester Nov 20, 2018

bobcatfish left a comment

vdemeester commented Nov 20, 2018

vdemeester commented Nov 21, 2018

nader-ziada commented Nov 21, 2018

vdemeester commented Nov 21, 2018

nader-ziada commented Nov 21, 2018

nader-ziada left a comment

knative-prow-robot commented Nov 22, 2018

nader-ziada commented Nov 22, 2018

vdemeester commented Nov 22, 2018

knative-metrics-robot commented Nov 22, 2018

nader-ziada commented Nov 22, 2018

Add Build status fields to TaskRun status #243

Add Build status fields to TaskRun status #243

Conversation

vdemeester commented Nov 19, 2018 • edited Loading

vdemeester commented Nov 20, 2018

vdemeester commented Nov 20, 2018

bobcatfish Nov 20, 2018

Choose a reason for hiding this comment

bobcatfish Nov 20, 2018

Choose a reason for hiding this comment

bobcatfish Nov 20, 2018

Choose a reason for hiding this comment

vdemeester Nov 20, 2018

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

vdemeester commented Nov 20, 2018

vdemeester commented Nov 21, 2018

nader-ziada commented Nov 21, 2018

vdemeester commented Nov 21, 2018

nader-ziada commented Nov 21, 2018

nader-ziada left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 22, 2018

nader-ziada commented Nov 22, 2018

vdemeester commented Nov 22, 2018

knative-metrics-robot commented Nov 22, 2018

nader-ziada commented Nov 22, 2018

vdemeester commented Nov 19, 2018 •

edited

Loading