Separate v1beta1.TaskObject to Task and ClusterTask in TaskRun Reconciler #6452

JeromeJu · 2023-03-29T12:16:52Z

Changes

This commit is WIP.
This commit separates v1beta1.TaskObject to Task and ClusterTask to get prepared for v1 storage swap.

part of #5979
/kind misc

I wish I did have a nicer way of breaking up v1beta1.TaskObject instead of returning one of the two CRDs. Any suggestions would be much appreciated

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

[n/a] Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
[n/a] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
[n/a] Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2023-03-29T12:16:54Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tekton-robot · 2023-03-29T12:16:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign pritidesai after the PR has been reviewed.
You can assign the PR to them by writing /assign @pritidesai in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

tekton-robot · 2023-03-29T12:23:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	86.8%	-2.2
pkg/reconciler/taskrun/resources/taskspec.go	100.0%	76.7%	-23.3
pkg/trustedresources/verify.go	95.2%	94.4%	-0.9

tekton-robot · 2023-03-29T12:24:16Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	86.8%	-2.2
pkg/reconciler/taskrun/resources/taskspec.go	100.0%	76.7%	-23.3
pkg/trustedresources/verify.go	95.2%	94.4%	-0.9

JeromeJu · 2023-03-29T14:02:33Z

pkg/reconciler/taskrun/resources/taskref_test.go

+				return
+			}
+
+			if d := cmp.Diff(resolvedTask, tc.expected); d != "" {


Could you please help me to understand why the nil pointers here are not asserted equal with nil tc.expected here even with the cmpopts.EquateEmpty?
cc @Yongxuanzhang

the previous test cases are all comparing with nil, so I think it should work?

Yep I also thought the same. but was stuck here for a bit :/

https://prow.tekton.dev/view/gs/tekton-prow/pr-logs/pull/tektoncd_pipeline/6452/pull-tekton-pipeline-unit-tests/1641051864147431424

need to change the expected from runtime.Object to *v1beta1.Task

Could we check if it makes sense to cast the expected nil as (*v1beta1.Task)(nil) while keeping the expected as runtime.Object for being more generic in the test case struct?

I think we could just use *v1beta1.Task

Taking a second look on this, I personally feel that maybe we should keep runtime.Object since tc.expected could also be a ClusterTaskKind even though that we do not necessarily need it since trustedResources won't verify ClusterTask. 🤔 please feel free to help correct if I'm mistaken

We changed VerifyTask's in this PR to pass task not taskobject, so in tests we should test task

Thanks for the pointer! updated.

tekton-robot · 2023-03-29T15:27:22Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	89.2%	0.1
pkg/reconciler/taskrun/resources/taskspec.go	100.0%	76.7%	-23.3
pkg/trustedresources/verify.go	95.2%	94.4%	-0.9

pkg/trustedresources/verify.go

tekton-robot · 2023-03-29T16:29:23Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	89.2%	0.1
pkg/reconciler/taskrun/resources/taskspec.go	100.0%	86.7%	-13.3
pkg/trustedresources/verify.go	95.2%	94.4%	-0.9

tekton-robot · 2023-03-29T16:32:41Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	89.2%	0.1
pkg/reconciler/taskrun/resources/taskspec.go	100.0%	86.7%	-13.3
pkg/trustedresources/verify.go	95.2%	94.4%	-0.9

tekton-robot · 2023-03-29T17:04:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	89.2%	0.1
pkg/reconciler/taskrun/resources/taskspec.go	100.0%	93.3%	-6.7
pkg/trustedresources/verify.go	95.2%	94.4%	-0.9

pkg/trustedresources/verify.go

tekton-robot · 2023-03-30T10:02:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	88.2%	-0.9
pkg/reconciler/taskrun/resources/taskspec.go	100.0%	93.3%	-6.7
pkg/trustedresources/verify.go	95.2%	95.3%	0.1

tekton-robot · 2023-03-30T12:29:21Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	88.2%	-0.9
pkg/trustedresources/verify.go	95.2%	95.3%	0.1

JeromeJu · 2023-03-30T12:53:37Z

The test coverage drop was resulted from separating the TaskObject, while since ClusterTask won't get verified as trustedResources, it would not be necessary to restore it with new ClusterTask test case?

tekton-robot · 2023-03-30T13:07:33Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	88.2%	-0.9
pkg/trustedresources/verify.go	95.2%	95.3%	0.1

tekton-robot · 2023-03-30T14:03:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	88.2%	-0.9
pkg/trustedresources/verify.go	95.2%	95.3%	0.1

tekton-robot · 2023-03-30T14:04:13Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	88.2%	-0.9
pkg/trustedresources/verify.go	95.2%	95.3%	0.1

lbernick · 2023-03-30T15:32:50Z

pkg/reconciler/taskrun/resources/taskref.go

-// v1beta1.TaskObject.
-func resolveTask(ctx context.Context, resolver remote.Resolver, name string, kind v1beta1.TaskKind, k8s kubernetes.Interface) (v1beta1.TaskObject, *v1beta1.ConfigSource, error) {
+// v1beta1.Task or v1beta1.ClusterTask
+func resolveTask(ctx context.Context, resolver remote.Resolver, name string, kind v1beta1.TaskKind, k8s kubernetes.Interface) (*v1beta1.Task, *v1beta1.ClusterTask, *v1beta1.ConfigSource, error) {


instead of returning a task and a clusterTask, could we instead convert a clusterTask definition into a Task and just return the Task?

yeah I also wanted to do so but I am not sure if there is a way to convert it back to a ClusterTask as specified at

pipeline/pkg/reconciler/taskrun/resources/taskref.go

Line 151 in 54c0f84

return resolveTask(ctx, resolver, name, kind, k8s)

without any extra flags?

Why do we need to convert back into a clustertask?

I think for the case of a remote-cluster-task, https://github.com/tektoncd/pipeline/blob/main/pkg/reconciler/taskrun/resources/taskref_test.go#L376-L407 the resolveTask and the upstream of its caller GetTaskFunc shall return a ClusterTask at the end?

Not sure about those tests, but we just need to fetch the task definition from the cluster, store the task spec in the taskrun status, and execute it. we don't need to keep track of the fact that it's a clustertask. If you take a look at resolveTask, only the name and task spec (not the task kind) are used from the results of getTask.

Thanks @lbernick , I updated the alternative at #6471, would be happy to close the current PR if #6471 is clearer.

One concern was that I am not fully sure when we swap all v1beta1 to v1, if it is still ideal handling ClusterTask? Would be more explicit having one more return value in the getTaskFunc while we are also converting it to v1.Task?

We can probably convert a v1beta1 ClusterTask spec to a v1 TaskSpec, so I'm not concerned about the proposed solution after swapping to v1.

tekton-robot · 2023-03-30T16:10:30Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	88.2%	-0.9
pkg/trustedresources/verify.go	95.2%	95.3%	0.1

tekton-robot · 2023-03-30T16:11:26Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.0%	95.4%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	88.2%	-0.9
pkg/trustedresources/verify.go	95.2%	95.3%	0.1

…iler This commit separates v1beta1.TaskObject to Task and ClusterTask to get prepared for v1 storage swap.

tekton-robot · 2023-03-31T02:48:53Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.3%	95.8%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	88.2%	-0.9
pkg/trustedresources/verify.go	95.2%	95.3%	0.1

tekton-robot · 2023-03-31T02:49:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.3%	95.8%	-0.5
pkg/reconciler/taskrun/resources/taskref.go	89.0%	88.2%	-0.9
pkg/trustedresources/verify.go	95.2%	95.3%	0.1

JeromeJu · 2023-03-31T17:04:27Z

closing #6471 as the alternative that is cleaner than the current PR
Thanks to the reviews @lbernick and @Yongxuanzhang .

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 29, 2023

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 29, 2023

tekton-robot requested review from lbernick and vdemeester March 29, 2023 12:17

tekton-robot added kind/misc Categorizes issue or PR as a miscellaneuous one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. labels Mar 29, 2023

JeromeJu commented Mar 29, 2023

View reviewed changes

JeromeJu force-pushed the v1task-object branch from 4d50b62 to 7954bc0 Compare March 29, 2023 15:19

JeromeJu commented Mar 29, 2023

View reviewed changes

pkg/trustedresources/verify.go Outdated Show resolved Hide resolved

JeromeJu force-pushed the v1task-object branch from 7954bc0 to 002730b Compare March 29, 2023 16:22

JeromeJu commented Mar 30, 2023

View reviewed changes

pkg/trustedresources/verify.go Outdated Show resolved Hide resolved

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2023

JeromeJu force-pushed the v1task-object branch from 537361c to 22b360b Compare March 30, 2023 09:55

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2023

JeromeJu force-pushed the v1task-object branch from 22b360b to c20b76a Compare March 30, 2023 12:22

JeromeJu changed the title ~~[WIP] Separate v1beta1.TaskObject to Task and ClusterTask in Reconciler~~ Separate v1beta1.TaskObject to Task and ClusterTask in TaskRun Reconciler Mar 30, 2023

JeromeJu marked this pull request as ready for review March 30, 2023 13:01

JeromeJu removed the request for review from vdemeester March 30, 2023 13:01

JeromeJu requested review from Yongxuanzhang and removed request for lbernick March 30, 2023 13:01

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 Mar 30, 2023

tekton-robot requested review from bobcatfish and dibyom March 30, 2023 13:01

JeromeJu force-pushed the v1task-object branch from c20b76a to d10d2d4 Compare March 30, 2023 13:43

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2023

JeromeJu force-pushed the v1task-object branch from d10d2d4 to b8c2462 Compare March 30, 2023 13:57

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2023

JeromeJu mentioned this pull request Mar 30, 2023

Change the Storage Version to V1 Types #6444

Merged

7 tasks

lbernick reviewed Mar 30, 2023

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2023

JeromeJu force-pushed the v1task-object branch from b8c2462 to 5136184 Compare March 30, 2023 16:04

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2023

Separate v1beta1.TaskObject to Task and ClusterTask in TaskRun Reconc…

ef6e8f2

…iler This commit separates v1beta1.TaskObject to Task and ClusterTask to get prepared for v1 storage swap.

JeromeJu force-pushed the v1task-object branch from 5136184 to ef6e8f2 Compare March 31, 2023 02:42

JeromeJu closed this Mar 31, 2023

Separate v1beta1.TaskObject to Task and ClusterTask in TaskRun Reconciler #6452

Separate v1beta1.TaskObject to Task and ClusterTask in TaskRun Reconciler #6452

Conversation

JeromeJu commented Mar 29, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

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

JeromeJu Mar 30, 2023 • 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

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 29, 2023

tekton-robot commented Mar 30, 2023

tekton-robot commented Mar 30, 2023

JeromeJu commented Mar 30, 2023

tekton-robot commented Mar 30, 2023

tekton-robot commented Mar 30, 2023

tekton-robot commented Mar 30, 2023

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

tekton-robot commented Mar 30, 2023

tekton-robot commented Mar 30, 2023

tekton-robot commented Mar 31, 2023

tekton-robot commented Mar 31, 2023

JeromeJu commented Mar 31, 2023

JeromeJu commented Mar 29, 2023 •

edited

Loading

JeromeJu Mar 30, 2023 •

edited

Loading