Deprecate PipelineRun.Spec.ServiceAccountNames 🦌 #3028

vdemeester · 2020-07-29T09:28:24Z

Changes

PipelineRun.Spec.TaskRunSpecs already allows to specify a
ServiceAccountName for a specific task. This marks
PipelineRun.Spec.ServiceAccountNames as deprecated and to be removed
in about 9 months.

This also make sure PipelineRun.Spec.ServiceAccountNames are not ignored.

Fixes #2614

Signed-off-by: Vincent Demeester vincent@sbr.pm

/cc @bobcatfish @sbwsg @afrittoli @jlpettersson

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
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

PipelineRun.Spec.ServiceAccountNames is being deprecated in favor of PipelineRun.Spec.TaskRunSpec[].ServiceAccountName

vdemeester · 2020-07-29T09:28:46Z

/kind cleanup

`PipelineRun.Spec.TaskRunSpecs` already allows to specify a `ServiceAccountName` for a specific task. This marks `PipelineRun.Spec.ServiceAccountNames` as deprecated and to be removed in about 9 months. This also make sure `PipelineRun.Spec.ServiceAccountNames` are not ignored. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

tekton-robot · 2020-07-29T09:40:18Z

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/pipelinerun.go	86.0%	85.6%	-0.4
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	90.2%	-2.5

jlpettersson

/lgtm

jlpettersson · 2020-07-29T09:42:42Z

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

+	for _, taskrunSpec := range pr.Spec.TaskRunSpecs {
+		if _, ok := pipelineTasks[taskrunSpec.PipelineTaskName]; !ok {
+			return fmt.Errorf("PipelineRun's taskrunSpecs defined wrong taskName: %q, does not exist in Pipeline", taskrunSpec.PipelineTaskName)
+		}


Nice to validate taskNames!

dlorenc · 2020-07-29T13:42:20Z

nice! lgtm, this is an API change though so I think it needs to be approved by "more than 50% of owners", right?

vdemeester · 2020-07-29T13:49:32Z

nice! lgtm, this is an API change though so I think it needs to be approved by "more than 50% of owners", right?

So… nothing in the API changes (aka no field is added or removed), we are just marking a field for deprecation (that we should have done when TaskRunSpecs was added ServiceAccountName support) ; so I am not fully sure if it meets the API changes requirement 😅. That said, I prefer to be safe than sorry 😝

/cc @bobcatfish @dibyom @afrittoli @imjasonh

bobcatfish · 2020-07-29T14:47:44Z

I agree with @dlorenc 's interpretation: i think that approving and merging this PR would be our intent to have a backwards incompatible change, even if we don't make that change in this PR.

I approve - i wonder if it might be worth taking some time to examine taskRunSpecs a bit more closely, it feels like we haven't though through that area of the API carefully (for example not noticing that it repeated the info in serviceAccountNames) but that might just be me being like "i wasnt part of this so i dont like it" so 🤷‍♀️ . I agree that having both doesn't make sense.

vdemeester · 2020-07-31T09:54:13Z

I agree with @dlorenc 's interpretation: i think that approving and merging this PR would be our intent to have a backwards incompatible change, even if we don't make that change in this PR.

I approve - i wonder if it might be worth taking some time to examine taskRunSpecs a bit more closely, it feels like we haven't though through that area of the API carefully (for example not noticing that it repeated the info in serviceAccountNames) but that might just be me being like "i wasnt part of this so i dont like it" so woman_shrugging . I agree that having both doesn't make sense.

So the gist behind TaskRunSpecs is that, we started with serviceAccountNames to specify serviceAccountName per tasks (in the pipeline). Then there was a need for doing the same for podTemplate. It made more sense to group those than having a field for each with the same "structure" (task, type).

afrittoli · 2020-08-02T08:41:32Z

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -366,14 +366,23 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err
 		return controller.NewPermanentError(err)
 	}

-	// Ensure that the ServiceAccountNames defined correct.
+	// Ensure that the ServiceAccountNames defined are correct.
+	// This is "deprecated".


NIT: why in quotes?

afrittoli · 2020-08-02T08:47:56Z

I approve of this change.
Counting @vdemeester and myself, the change is approved by 50% of the owners, which is not enough to meet the rule yet.
I think it's not a big deal if this shifts to v0.16.0 so I'll stick to the rule and not merge this for now.
@vdemeester you'll have to update the deprecation notices from v0.15.0 to v0.16.0.

@pritidesai @sbwsg FYI

ghost · 2020-08-03T14:12:36Z

Yup, I also approve.

dlorenc · 2020-08-03T18:59:46Z

Looks like we're at quorom now!

/approve

tekton-robot · 2020-08-03T18:59:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dlorenc, jlpettersson

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

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 afrittoli, bobcatfish, jlpettersson and a user July 29, 2020 09:28

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 29, 2020

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jul 29, 2020

vdemeester force-pushed the 2614-serviceAccountNames-deprecation branch from 2994728 to 2354446 Compare July 29, 2020 09:30

vdemeester force-pushed the 2614-serviceAccountNames-deprecation branch from 2354446 to e6fe3bf Compare July 29, 2020 09:37

vdemeester added this to the Pipelines v0.15 milestone Jul 29, 2020

jlpettersson approved these changes Jul 29, 2020

View reviewed changes

tekton-robot assigned jlpettersson Jul 29, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020

tekton-robot requested review from dibyom and imjasonh July 29, 2020 13:49

afrittoli reviewed Aug 2, 2020

View reviewed changes

vdemeester modified the milestones: Pipelines v0.15, Pipelines v0.16 Aug 3, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2020

tekton-robot merged commit 28d1347 into tektoncd:master Aug 3, 2020

vdemeester deleted the 2614-serviceAccountNames-deprecation branch August 4, 2020 08:11

Yongxuanzhang mentioned this pull request Aug 15, 2023

fix: refine error resean with invalid pipelinename in taskrunspecs #6957

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate PipelineRun.Spec.ServiceAccountNames 🦌 #3028

Deprecate PipelineRun.Spec.ServiceAccountNames 🦌 #3028

vdemeester commented Jul 29, 2020 •

edited by dlorenc

vdemeester commented Jul 29, 2020

tekton-robot commented Jul 29, 2020

jlpettersson left a comment

jlpettersson Jul 29, 2020

dlorenc commented Jul 29, 2020

vdemeester commented Jul 29, 2020

bobcatfish commented Jul 29, 2020

vdemeester commented Jul 31, 2020

afrittoli Aug 2, 2020

afrittoli commented Aug 2, 2020

ghost commented Aug 3, 2020

dlorenc commented Aug 3, 2020

tekton-robot commented Aug 3, 2020

Deprecate PipelineRun.Spec.ServiceAccountNames 🦌 #3028

Deprecate PipelineRun.Spec.ServiceAccountNames 🦌 #3028

Conversation

vdemeester commented Jul 29, 2020 • edited by dlorenc

Changes

Submitter Checklist

Reviewer Notes

Release Notes

vdemeester commented Jul 29, 2020

tekton-robot commented Jul 29, 2020

jlpettersson left a comment

Choose a reason for hiding this comment

jlpettersson Jul 29, 2020

Choose a reason for hiding this comment

dlorenc commented Jul 29, 2020

vdemeester commented Jul 29, 2020

bobcatfish commented Jul 29, 2020

vdemeester commented Jul 31, 2020

afrittoli Aug 2, 2020

Choose a reason for hiding this comment

afrittoli commented Aug 2, 2020

ghost commented Aug 3, 2020

dlorenc commented Aug 3, 2020

tekton-robot commented Aug 3, 2020

vdemeester commented Jul 29, 2020 •

edited by dlorenc