Removes "build" word references from TaskRun #818

hrishin · 2019-04-30T19:34:20Z

Changes

At present TaskRun's status, it's pod containers name(step name
prefix) and logs have references to build keyword.
Which kind of gives the perception that Task is intended to
perform only build operations.

This patch removes those references from TaskRun.

Fixes #815

Submitter Checklist

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

Release Notes

🚨 Remove "build" word references from TaskRun in the steps container name.

dlorenc · 2019-04-30T20:03:44Z

/ok-to-test

hrishin · 2019-05-01T03:01:44Z

/retest

vdemeester

Interesting failure…

-: v1alpha1.StepState{ContainerState: v1.ContainerState{Terminated: s"&ContainerStateTerminated{ExitCode:0,Signal:0,Reason:Completed,Message:,StartedAt:2019-05-01 03:12:42 +0000 UTC,FinishedAt:2019-05-01 03:12:42 +0000 UTC,ContainerID:docker://46c79150e7ee174883921feb7619e42ba9c63d5c2e8285233f88e917e18b892a,}"}, Name: "nop"}
+: <non-existent>
    {[]v1alpha1.StepState}[?->3]:
-: <non-existent>
+: v1alpha1.StepState{ContainerState: v1.ContainerState{Terminated: s"&ContainerStateTerminated{ExitCode:0,Signal:0,Reason:Completed,Message:,StartedAt:0001-01-01 00:00:00 +0000 UTC,FinishedAt:0001-01-01 00:00:00 +0000 UTC,ContainerID:,}"}, Name: "nop"}

hrishin · 2019-05-01T10:28:51Z

yeah I'm not getting exactly, looks like this PR has messed with nop step

hrishin · 2019-05-07T15:30:34Z

/test pull-tekton-pipeline-integration-tests

aristosvo

It looks like the order of steps is changed. In the failing test the order is:

exit
hello
world
nop

In the output yaml the order is:

nop
exit
hello
world

googlebot · 2019-05-29T15:49:43Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-29T15:54:07Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

vdemeester · 2019-05-29T17:33:48Z

cc @bobcatfish @abayer @dlorenc

hrishin · 2019-05-29T18:20:04Z

test/taskrun_test.go

 			},
 		},
-		Name: "exit",
+		Name: "nop",


Just wondering if step order should change?
Now as per this fix nop step is appearing at the top. If the user fetches the logs it would show the nop step first?
Is it desired? 🤔

Good question. I feel it's no big deal (as the order was always alphabitically and not from the step order), but… @bobcatfish 🙏

it doesn't seem like we have any control over this, so I don't think it matters (does anyone understand why these are in the order they're in? it seems pretty random!)

yeah its because of taskrun reconciler copies the pod.status.containerStatus for all steps. By default, k8s sort the containerStatus in alphabetically sorted order (which is wired don't know why).

While working on fetching the task logs, one has to either iterate through pod.status.containerStatus or TaskRun.Status.Steps. Now the issue is, steps order is not maintained anymore, it has to find the currently running container and rather simply iterate over taskrun.status.steps while printing the logs. isn't it wired? 🤔

it doesn't seem like we have any control over this

One approach is while updating the taskrun status, we can sort containers status as per task.spec.steps order. 🤔

WDYT? @vdemeester @bobcatfish

hm I guess we could sort them - I expect we'd probably want to have the order of the statuses match the order that the steps are declared in?

anyway my feeling is that we do this in a separate issue and PR, if there are no objections! (I wouldn't think of the order here as part of the interface)

Created an issue to follow-up #942

pkg/reconciler/v1alpha1/taskrun/resources/pod.go

bobcatfish · 2019-05-29T22:32:46Z

test/taskrun_test.go

 			},
 		},
-		Name: "exit",
+		Name: "nop",


it doesn't seem like we have any control over this, so I don't think it matters (does anyone understand why these are in the order they're in? it seems pretty random!)

At present `TaskRun's` `status`, it's pod containers name(`step name` prefix) and logs has references to `build` keyword. Which kind of gives the perception that `Task` is intended to perform only build operations. This patch removes those references from `TaskRun`. Fixes - tektoncd#815 Signed-off-by: Vincent Demeester <[email protected]>

vdemeester · 2019-06-03T07:53:32Z

@bobcatfish updated 👼

bobcatfish · 2019-06-03T21:48:19Z

Looks good to me!

/lgtm

@vdemeester I'll leave it up to you if you want to address the container status issue (I wasn't clear on whether or not we were all in agreement about leaving it as is)

/hold

/meow space

tekton-robot · 2019-06-03T21:48:25Z

@bobcatfish:

In response to this:

Looks good to me!

/lgtm

@vdemeester I'll leave it up to you if you want to address the container status issue (I wasn't clear on whether or not we were all in agreement about leaving it as is)

/hold

/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester

/approve
/hold cancel

tekton-robot · 2019-06-04T06:49:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hrishin, 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~~ [vdemeester]

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

vdemeester · 2019-06-04T09:38:48Z

/test pull-tekton-pipeline-integration-tests

hrishin · 2019-06-04T10:51:09Z

/retest

vdemeester · 2019-06-04T12:56:52Z

/test pull-tekton-pipeline-integration-tests

tekton-robot requested a review from dlorenc April 30, 2019 19:34

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 30, 2019

tekton-robot requested a review from imjasonh April 30, 2019 19:34

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 30, 2019

tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 30, 2019

vdemeester reviewed May 1, 2019

View reviewed changes

hrishin force-pushed the fix-815 branch from 14d476b to 51a9d0e Compare May 7, 2019 13:24

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 7, 2019

aristosvo suggested changes May 24, 2019

View reviewed changes

vdemeester force-pushed the fix-815 branch 2 times, most recently from 66f228d to b9b44e9 Compare May 29, 2019 15:49

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels May 29, 2019

vdemeester force-pushed the fix-815 branch from b9b44e9 to 1c1ce14 Compare May 29, 2019 15:54

googlebot added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels May 29, 2019

vdemeester force-pushed the fix-815 branch from 1c1ce14 to 5e530f3 Compare May 29, 2019 17:17

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 29, 2019

hrishin commented May 29, 2019

View reviewed changes

bobcatfish reviewed May 29, 2019

View reviewed changes

vdemeester force-pushed the fix-815 branch from 5e530f3 to 58ed222 Compare May 30, 2019 06:50

tekton-robot assigned bobcatfish Jun 3, 2019

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 3, 2019

vdemeester mentioned this pull request Jun 4, 2019

Order TaskRun.Status.Steps same way as Task.Spec.Steps #942

Closed

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2019

vdemeester approved these changes Jun 4, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2019

tekton-robot merged commit 3d87e2e into tektoncd:master Jun 4, 2019

vdemeester mentioned this pull request Jun 6, 2019

Proposal: Tekton objects should be self descriptive using annotations instead of insider info (container name prefix...) #961

Closed

bobcatfish mentioned this pull request Jun 13, 2019

Formalize review requirements for API changes #906

Closed

AlanGreene mentioned this pull request Jul 11, 2019

Error loading pipelines tektoncd/dashboard#303

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes "build" word references from TaskRun #818

Removes "build" word references from TaskRun #818

hrishin commented Apr 30, 2019 •

edited by vdemeester

Loading

dlorenc commented Apr 30, 2019

hrishin commented May 1, 2019

vdemeester left a comment

hrishin commented May 1, 2019

hrishin commented May 7, 2019

aristosvo left a comment

googlebot commented May 29, 2019

googlebot commented May 29, 2019

vdemeester commented May 29, 2019

hrishin May 29, 2019

vdemeester May 29, 2019

bobcatfish May 29, 2019

hrishin May 31, 2019 •

edited

Loading

bobcatfish Jun 3, 2019

vdemeester Jun 4, 2019

bobcatfish May 29, 2019

vdemeester commented Jun 3, 2019

bobcatfish commented Jun 3, 2019

tekton-robot commented Jun 3, 2019

vdemeester left a comment

tekton-robot commented Jun 4, 2019

vdemeester commented Jun 4, 2019

hrishin commented Jun 4, 2019

vdemeester commented Jun 4, 2019

Removes "build" word references from TaskRun #818

Removes "build" word references from TaskRun #818

Conversation

hrishin commented Apr 30, 2019 • edited by vdemeester Loading

Changes

Submitter Checklist

Release Notes

dlorenc commented Apr 30, 2019

hrishin commented May 1, 2019

vdemeester left a comment

Choose a reason for hiding this comment

hrishin commented May 1, 2019

hrishin commented May 7, 2019

aristosvo left a comment

Choose a reason for hiding this comment

googlebot commented May 29, 2019

googlebot commented May 29, 2019

vdemeester commented May 29, 2019

hrishin May 29, 2019

Choose a reason for hiding this comment

vdemeester May 29, 2019

Choose a reason for hiding this comment

bobcatfish May 29, 2019

Choose a reason for hiding this comment

hrishin May 31, 2019 • edited Loading

Choose a reason for hiding this comment

bobcatfish Jun 3, 2019

Choose a reason for hiding this comment

vdemeester Jun 4, 2019

Choose a reason for hiding this comment

bobcatfish May 29, 2019

Choose a reason for hiding this comment

vdemeester commented Jun 3, 2019

bobcatfish commented Jun 3, 2019

tekton-robot commented Jun 3, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 4, 2019

vdemeester commented Jun 4, 2019

hrishin commented Jun 4, 2019

vdemeester commented Jun 4, 2019

hrishin commented Apr 30, 2019 •

edited by vdemeester

Loading

hrishin May 31, 2019 •

edited

Loading