Sort the steps of taskrun based on the steps of task #963

houshengbo · 2019-06-06T19:19:24Z

Changes

Closes: #942

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

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

See the contribution guide
for more details.

Release Notes

release-note

houshengbo · 2019-06-06T19:22:50Z

/assign @hrishin @bobcatfish @vdemeester

chmouel · 2019-06-07T08:12:29Z

this sounds worthwhile enough to mention in the release note,

vdemeester · 2019-06-07T08:39:59Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+func sortTaskRunSteps(taskRunSteps []v1alpha1.StepState, taskSpecSteps []corev1.Container) {
+	// Generate the map matching the step name and its index
+	sorter := constructTaskStepsSorter(taskSpecSteps)
+	for index, step := range taskRunSteps {


We may be able to use the sort package here 👼

I tried, but the the sort package is easy to sort the array with a filed or multiple fields in itself. Have not figured out to sort one array by aligning with another array.

vdemeester · 2019-06-07T08:41:06Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

+
+	sortTaskRunSteps(taskRunStatusSteps, task.Spec.Steps)
+
+	for index, step := range task.Spec.Steps {


I wonder if it would be more clear to do the cmp.Diff on the taskRunStatusSteps and an expectedTaskRunStatusSteps instead 👼

My question is how to create the fake taskRunStatusSteps and the expectedTaskRunStatusSteps?

well, expectedTaskRunStatusStep would be declared as taskRunStatusSteps but in the right order 👼

hrishin

Thank you @houshengbo 😄 for the PR.
Left few comments, please me know if that makes sense? 🙇‍♂️

hrishin · 2019-06-10T18:03:06Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -361,6 +415,12 @@ func updateStatusFromPod(taskRun *v1alpha1.TaskRun, pod *corev1.Pod, resourceLis
 		})
 	}

+	if len(taskSpecSteps) > 0 {


wondering if this check is really needed? 🤔

I change the code structure a bit in order to separate the sortTaskRun function.

hrishin · 2019-06-10T18:06:20Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	if len(taskSpecSteps) > 0 {
+		trt := new(TaskRunWithTask)
+		trt.Init(taskRun.Status.Steps, taskSpecSteps[0])
+		sort.Sort(trt)


Wondering if sorting should be done outside of updateStatusFromPod method? 🤔
In that way

Writing the unit test would be easy

Single responsibility: Sorting responsibility could be taken outside of methods scope

I change the code structure a bit in order to separate the sortTaskRun function.

houshengbo · 2019-06-11T14:30:49Z

@hrishin I have changed the structure of the code a bit. In order to separate the sort function, we have to make sure it is done between we get the taskrun and we update the taskrun.

houshengbo · 2019-06-11T14:54:07Z

/retest

hrishin · 2019-06-11T15:02:53Z

Thank you @houshengbo 🙇‍♂️

vdemeester

One nit, otherwise looks good
/lgtm

vdemeester · 2019-06-12T13:37:04Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	mapForSort map[string]int
+}
+
+func (trt *TaskRunWithTask) Init(taskRunSteps []v1alpha1.StepState, taskSpecSteps []corev1.Container) {


nit: I feel we don't need this and we could just declare it inline here

trt := &TaskRunWithTask{ taskRunSteps: taskRunSteps, mapForSort: trt.constructTaskStepsSorter(taskSpecSteps), }

But it's more a aesthetic thing than anything, so I am ok with both I think 👼

bobcatfish

Thanks for working on this @houshengbo !! I have a couple of requests:

Could you also add some docs to https://github.com/tektoncd/pipeline/blob/master/docs/taskruns.md#syntax about the states in the status and what order they are expected to be in?
Could you add a test at the reconciler level as well (https://github.com/tektoncd/pipeline/blob/3d87e2e18174f1d7b088bdd14065d5f06ef86462/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go) and also update an end to end test to verify this? Having this work correctly relies on the sorting function being called in the reconciler and then no more modifications being made to the status order after that, which future contributors might not realize, so adding some more testing around this could save us later :)

bobcatfish · 2019-06-12T22:54:06Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	// elements of the array in the Swap function.
+	return true
+}
+


can we move this logic out of the reconciler into a separate file / separate package? 🙏 the less code in the reconciler the better :D

This would include moving sortTaskRunStepOrder and making it exported in the new package (SortTaskRunStepOrder)

I thought about it before.

Would you mind we have a new package with a more general name like utils, so that we can put this sorter and maybe further algorithm(TBD) inside it? @bobcatfish

I'd like to name a new package utils.

@houshengbo I would definitely advise against utils 😅 generic names like utils or helpers do not convey anything of what is inside. This function is about sorting taskrun, so it could either be called sort (inside taskrun) or something a tiny bit more general like list ?

Forget about my previous comment.
what about stepsorting

@houshengbo 🤗 as it's related to tasks I feel it would be better in a package name that has task in (the path or the name) (tasks.SortBySpecOrder instead of my previous propositon)

cc @bobcatfish as I am not the greatest to find correct names 😂

ooo i like both the ideas! stepsorting is pretty close to what I would have suggested but I think @vdemeester is right, we'd end up with something like stepsorting.Sort 🤔

we have been kind of in need of a status related package for a while now (we have lots have status stuff in the reconciler) how about something like taskrunstatus or status, so we could have taskrunstatus.SortSteps or status.SortSteps?

ohh I do like the status package 😻 status.SortSteps sounds really good to me !

yep, I think we can agree on that.

bobcatfish · 2019-06-12T22:57:15Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	trt.mapForSort = trt.constructTaskStepsSorter(taskSpecSteps)
+}
+
+// This function constructTaskStepsSorter constructs a map matching the names of


the expected format for comments like this i usually for them to start with the name of the function, e.g.:

// constructTaskStepsSorter constructs a map matching the names of ...

bobcatfish · 2019-06-12T23:00:30Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

+	}}
+
+	sortTaskRunStepOrder(taskRunStatusSteps, task.Spec.Steps)
+	if d := cmp.Diff(taskRunStatusSteps, expectedStepState); d != "" {


this is pretty minor but the test would be a bit easier to read if it asserted the names of the names of the steps only, e.g. something like:

actualStepOrder := []string{} for _, state := range(taskRunStatusSteps) { actualStepOrder = append(actualStepOrder, state.name) } expectedStepOrder := []string{"hello", "exit", "world", "nop"} if d := cmp.Diff(actualStepOrder, expectedStepOrder); d != "" { }

Then you can completely get rid of expectedStepState :D

(this would also make it a bit easier to add a few more tests cases as well)

bobcatfish · 2019-06-12T23:08:43Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -82,6 +83,58 @@ const (
 	imageDigestExporterContainerName = "step-image-digest-exporter"
 )

+type TaskRunWithTask struct {


I got a bit confused about what this type was - it seems to me like the main goal of this type is to sort StepStates, how about calling it something like StepStateSorter?

vdemeester

/lgtm

bobcatfish

Looking good! I just have a minor request about docs, otherwise good to go 🙏

bobcatfish · 2019-06-14T17:44:19Z

docs/taskruns.md

+If multiple `steps` are defined in the `Task` invoked by the `TaskRun`, we will see the
+`status.steps` of the `TaskRun` displayed in the same order as they are defined in
+`spec.steps` of the `Task`, when the `TaskRun` is accessed by the `get` command, e.g.
+`kubectl get taskrun <name> -o yaml`. Replace \<name\> with the name of the `TaskRun`.


thanks for adding this! could you put it in a status section on it's own, maybe between the syntax and ordering sections, and add a link to the new section in the overview at the top of the file? https://github.com/tektoncd/pipeline/blob/master/docs/pipelines.md#pipelines

This will give us a place to start putting more docs about the status in the future as well.

This is a change on the file taskruns.md, not pipeline.md.
Do you actually mean to add a section in taskruns.md?

whoops my bad! no taskruns is the right place :D

bobcatfish · 2019-06-14T17:44:48Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

+
+	// The order of the container statuses has been shuffled, not aligning with the order of the
+	// spec steps of the Task any more. After Reconcile is called, we should see the order of status
+	// steps in TaksRun has been converted to the same one as in spec steps of the Task.


nice comment!

bobcatfish · 2019-06-14T17:46:31Z

pkg/status/stepstatesorter.go

+	trt := new(StepStateSorter)
+	trt.Init(taskRunSteps, taskSpecSteps)
+	sort.Sort(trt)
+}


nice, thanks for making the new package!

bobcatfish · 2019-06-14T17:46:54Z

pkg/status/stepstatesorter_test.go

+		actualStepOrder = append(actualStepOrder, state.Name)
+	}
+
+	expectedStepOrder := []string{"hello", "exit", "world", "nop"}


nice, i like how the expected order is hardcoded here, very clear!

bobcatfish · 2019-06-14T18:04:40Z

I0614 17:21:03.378] 2019/06/14 17:21:03 util.go:142: Please use kubetest --gcp-service-account=/etc/test-account/service-account.json (instead of deprecated GOOGLE_APPLICATION_CREDENTIALS=/etc/test-account/service-account.json)
I0614 17:21:03.379] 2019/06/14 17:21:03 main.go:761: --gcp-project is missing, trying to fetch a project from boskos.
I0614 17:21:03.379] (for local runs please set --gcp-project to your dev project)
I0614 17:21:03.379] 2019/06/14 17:21:03 main.go:773: provider gke, will acquire project type gke-project from boskos
I0614 17:21:03.434] 2019/06/14 17:21:03 main.go:312: Something went wrong: failed to prepare test environment: --provider=gke boskos failed to acquire project: resource not found
I0614 17:21:03.462] Test subprocess exited with code 0

bah, reopened tektoncd/plumbing#29

/test pull-tekton-pipeline-integration-tests

bobcatfish · 2019-06-14T18:21:56Z

I0614 18:16:31.836] 2019/06/14 18:16:31 error processing import paths in "config/controller.yaml": Get https://gcr.io/v2/: net/http: TLS handshake timeout

whyyyyyyyy

bobcatfish · 2019-06-14T18:22:00Z

/test pull-tekton-pipeline-integration-tests

bobcatfish · 2019-06-14T19:12:53Z

/lgtm
/meow space

tekton-robot · 2019-06-14T19:12:54Z

@bobcatfish:

In response to this:

/lgtm
/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.

houshengbo · 2019-06-14T20:41:39Z

@bobcatfish Thank you.

bobcatfish · 2019-06-14T22:42:21Z

/lgtm

bobcatfish · 2019-06-14T22:46:42Z

/approve

tekton-robot · 2019-06-14T22:46:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, houshengbo

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~~ [bobcatfish]

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

tekton-robot requested review from bobcatfish and dlorenc June 6, 2019 19:19

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

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 6, 2019

tekton-robot assigned bobcatfish, hrishin and vdemeester Jun 6, 2019

vdemeester reviewed Jun 7, 2019

View reviewed changes

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 Jun 7, 2019

hrishin reviewed Jun 10, 2019

View reviewed changes

vdemeester mentioned this pull request Jun 11, 2019

Add first draft of Knative Build migration guide #976

Merged

vdemeester reviewed Jun 12, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2019

bobcatfish reviewed Jun 12, 2019

View reviewed changes

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2019

houshengbo changed the title ~~Sort the steps of taskrun based on the steps of task~~ WIP: Sort the steps of taskrun based on the steps of task Jun 13, 2019

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2019

bobcatfish mentioned this pull request Jun 13, 2019

Graceful sidecar support #936

Merged

3 tasks

vdemeester reviewed Jun 14, 2019

View reviewed changes

tekton-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jun 14, 2019

houshengbo changed the title ~~WIP: Sort the steps of taskrun based on the steps of task~~ Sort the steps of taskrun based on the steps of task Jun 14, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2019

bobcatfish reviewed Jun 14, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2019

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2019

Sort the steps of taskrun based on the steps of task

4777b50

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2019

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

tekton-robot merged commit 8552f8b into tektoncd:master Jun 14, 2019


		sortTaskRunSteps(taskRunStatusSteps, task.Spec.Steps)

		for index, step := range task.Spec.Steps {

Sort the steps of taskrun based on the steps of task #963

Sort the steps of taskrun based on the steps of task #963

Conversation

houshengbo commented Jun 6, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

houshengbo commented Jun 6, 2019

chmouel commented Jun 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrishin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houshengbo commented Jun 11, 2019

houshengbo commented Jun 11, 2019

hrishin commented Jun 11, 2019

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houshengbo Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Jun 14, 2019

bobcatfish commented Jun 14, 2019

bobcatfish commented Jun 14, 2019

bobcatfish commented Jun 14, 2019

tekton-robot commented Jun 14, 2019

houshengbo commented Jun 14, 2019

bobcatfish commented Jun 14, 2019

bobcatfish commented Jun 14, 2019

tekton-robot commented Jun 14, 2019

houshengbo commented Jun 6, 2019 •

edited

Loading

houshengbo Jun 13, 2019 •

edited

Loading

bobcatfish Jun 13, 2019 •

edited

Loading