[WIP] TEP of adding pipelinerun.spec.taskrunspectemplate

[yuzp1996]

[yuzp1996]

Related Issue: https://github.com/tektoncd/pipeline/issues/5302

PR is not ready for review now! 😂

[lbernick]

/kind tep

[dibyom]

/assign @lbernick

[jerop]

@tektoncd/core-maintainers need a non-googler assignee, please take a look

[yuzp1996]

Thanks for your reply @vdemeester !

Given that a TaskRun has a PodTemplate field, and that it is possible to set the PodTemplate in the TaskRunTemplate field, we should probably deprecate PodTemplate (not proposed in the TEP as far as I see).

Do you mean because PipelineRun has PodTemplate field, we should deprecate PodTemplate in TaskRunTemplate? If so I think It is a good idea since we don't need give users more approaches to do the same thing.

• Why do we "repeat" task in all field ? taskRunTemplate.serviceAccountName is good enough, and closer to what we set on a TaskRun, so why would we use taskServiceAccountName ? Also, why some have a task prefix and some not ?

Those fields are referenced from struct PipelineTaskRunSpec but it is my fault that I used them without thinking. These fields should not contain prefix tasks as it looks a bit verbose.

• The ServiceAccountName is actually not the best of examples, as.. PipelineRun already has a ServiceAccountName that affects all the TaskRun by default. We may need to evaluate API consistency here and reduce the number of ways we can provide "defaults" for all TaskRun from a PipelineRun except a few. Today, PodTemplate and ServiceAccountName are set on PipelineRun and apply by default to all created TaskRun.

Yes, I found the related code that PipelineRun's ServiceAccountName and Podtemplate applied to all of the TaskRun as the default that we do not need to provide another same way in TaskRunTemplate.

• This is not true for the rest of the proposed fields : StepOverrides, SidecarOverrides, ComputeResources and Metadata (?). Either we deprecated "root" fields like ServiceAccountName and PodTemplate and rely entirely on TaskRunTemplate, or we don't provide TaskRunTemplate and instead we add new PipelineRun fields for "all TaskRuns" StepOverrides, SidecarOverrides, ComputeResources, …

Of the two ways, I prefer the first way, which is to deprecate the "root" field and rely entirely on TaskRunTemplate, but it looks like it will bring some breaking changes. I'm not sure if it's worth it.

@vdemeester @lbernick What do you think?

[lbernick]

Given that a TaskRun has a PodTemplate field, and that it is possible to set the PodTemplate in the TaskRunTemplate field, we should probably deprecate PodTemplate (not proposed in the TEP as far as I see).

Do you mean because PipelineRun has PodTemplate field, we should deprecate PodTemplate in TaskRunTemplate? If so I think It is a good idea since we don't need give users more approaches to do the same thing.

I think Vincent actually meant deprecating pipelineRun.spec.podTemplate, because otherwise users wouldn't be able to specify a different podTemplate for each TaskRun -- @vdemeester correct me if I'm wrong.

• Why do we "repeat" task in all field ? taskRunTemplate.serviceAccountName is good enough, and closer to what we set on a TaskRun, so why would we use taskServiceAccountName ? Also, why some have a task prefix and some not ?

Those fields are referenced from struct PipelineTaskRunSpec but it is my fault that I used them without thinking. These fields should not contain prefix tasks as it looks a bit verbose.

I agree with Vincent here, and in fact we are going to rename these fields in v1 -- see https://github.com/tektoncd/community/pull/732

• The ServiceAccountName is actually not the best of examples, as.. PipelineRun already has a ServiceAccountName that affects all the TaskRun by default. We may need to evaluate API consistency here and reduce the number of ways we can provide "defaults" for all TaskRun from a PipelineRun except a few. Today, PodTemplate and ServiceAccountName are set on PipelineRun and apply by default to all created TaskRun.

Yes, I found the related code that PipelineRun's ServiceAccountName and Podtemplate applied to all of the TaskRun as the default that we do not need to provide another same way in TaskRunTemplate.

This is on me-- I mentioned the service account name example without digging into it enough. I think it would be reasonable to swap out pipelineRun.spec.serviceAccountName for pipelineRun.spec.taskRunTemplate.serviceAccountName if we go down this route.

• This is not true for the rest of the proposed fields : StepOverrides, SidecarOverrides, ComputeResources and Metadata (?). Either we deprecated "root" fields like ServiceAccountName and PodTemplate and rely entirely on TaskRunTemplate, or we don't provide TaskRunTemplate and instead we add new PipelineRun fields for "all TaskRuns" StepOverrides, SidecarOverrides, ComputeResources, …

Of the two ways, I prefer the first way, which is to deprecate the "root" field and rely entirely on TaskRunTemplate, but it looks like it will bring some breaking changes. I'm not sure if it's worth it.

I also prefer TaskRunTemplate. One option is to make these changes in v1, but I want to avoid having only the new fields in v1 and then realizing there was an issue we haven't thought of-- @vdemeester wdyt?

[vdemeester]

Given that a TaskRun has a PodTemplate field, and that it is possible to set the PodTemplate in the TaskRunTemplate field, we should probably deprecate PodTemplate (not proposed in the TEP as far as I see).

Do you mean because PipelineRun has PodTemplate field, we should deprecate PodTemplate in TaskRunTemplate? If so I think It is a good idea since we don't need give users more approaches to do the same thing.

I think Vincent actually meant deprecating pipelineRun.spec.podTemplate, because otherwise users wouldn't be able to specify a different podTemplate for each TaskRun -- @vdemeester correct me if I'm wrong.

I indeed mean to either deprecate PipelineRun.PodTemplate here, or not adding TaskRunTemplate and have root fields (below).

• The ServiceAccountName is actually not the best of examples, as.. PipelineRun already has a ServiceAccountName that affects all the TaskRun by default. We may need to evaluate API consistency here and reduce the number of ways we can provide "defaults" for all TaskRun from a PipelineRun except a few. Today, PodTemplate and ServiceAccountName are set on PipelineRun and apply by default to all created TaskRun.

Yes, I found the related code that PipelineRun's ServiceAccountName and Podtemplate applied to all of the TaskRun as the default that we do not need to provide another same way in TaskRunTemplate.

This is on me-- I mentioned the service account name example without digging into it enough. I think it would be reasonable to swap out pipelineRun.spec.serviceAccountName for pipelineRun.spec.taskRunTemplate.serviceAccountName if we go down this route.

I don't have a super strong opinion on this, I just want us to be consistent there and not have too much way to do the same thing. Having fields on root is simpler to write for users, but having them all under a TaskRunTemplate has also its benefit (duck typing, …). In addition, most PipelineRun are written by tooling so, I don't think verbosity would be too much of a problem (if we go with pipelineRun.spec.taskRunTemplate.{}. The fact that we have PodTemplate, StepTemplate, … would tend to lean towards using a taskRunTemplate 🙃.

• This is not true for the rest of the proposed fields : StepOverrides, SidecarOverrides, ComputeResources and Metadata (?). Either we deprecated "root" fields like ServiceAccountName and PodTemplate and rely entirely on TaskRunTemplate, or we don't provide TaskRunTemplate and instead we add new PipelineRun fields for "all TaskRuns" StepOverrides, SidecarOverrides, ComputeResources, …

Of the two ways, I prefer the first way, which is to deprecate the "root" field and rely entirely on TaskRunTemplate, but it looks like it will bring some breaking changes. I'm not sure if it's worth it.

I also prefer TaskRunTemplate. One option is to make these changes in v1, but I want to avoid having only the new fields in v1 and then realizing there was an issue we haven't thought of-- @vdemeester wdyt?

So I understand the worry, but I do not share it 😝. I think this is a relatively easy problem here as we are not "changing" behavior or creating new feature but more deciding where some information should be : either grouped under a field taskRunTemplate or directly on root. In addition, in a v1 it easier to "add fields" than removing them (quite litteraly impossible) — so I'd rather "forget" one that having too much 😬.

[lbernick]

So I understand the worry, but I do not share it 😝. I think this is a relatively easy problem here as we are not "changing" behavior or creating new feature but more deciding where some information should be : either grouped under a field taskRunTemplate or directly on root. In addition, in a v1 it easier to "add fields" than removing them (quite litteraly impossible) — so I'd rather "forget" one that having too much 😬.

Ok that's fair enough-- I'd be in favor of making this change in v1 then

[pritidesai]

/assign @vdemeester

[yuzp1996]

@vdemeester @lbernick looks like making this change in v1 is a better option, should I close this PR or finish this TEP first but implement it in v1?

[lbernick]

@yuzp1996 would you mind updating this TEP to address moving podTemplate and serviceAccountName under taskRunTemplate, and clarifying that the changes will be made in v1 only? Thanks :)

[yuzp1996]

I don't mind at all 😁. I'll do it when I'm fine.

[yuzp1996]

@vdemeester @lbernick Changes are synced, please correct me if I don't understand correctly.

[lbernick]

this looks good to me! I'll let vincent take another look

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, 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:

• ~~teps/OWNERS~~ [lbernick,vdemeester]

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

[lbernick]

/lgtm

[lbernick]

FYI @jerop @abayer I realized this may affect pipelines in pipelines since the TEP states that pipelinerun.spec.serviceaccountname can be passed to child PipelineRuns. With serviceaccountname moved under taskRunTemplate, this might make less sense. Maybe when we explore creating pipelineRun.spec.pipelineRunSpecs we can also explore pipelineRun.spec.pipelineRunTemplate?

[jerop]

sure, let's gather initial feedback on taskRunTemplate and then explore adding customRunTemplate and pipelineRunTemplate

also noticed the title calls it taskrunspectemplate but the tep itself is taskruntemplate -- which one is it? (i prefer the latter)

[lbernick]

also noticed the title calls it taskrunspectemplate but the tep itself is taskruntemplate -- which one is it? (i prefer the latter)

it's the latter