feat: support for variable interpolation in workspace.* (in PipelineRun and TaskRun) #7671

l-qing · 2024-02-15T06:43:00Z

fix #7667

Support for variable interpolation in:

spec.workspaces[].configMap.items[].*
spec.workspaces[].secret.items[].*
spec.workspaces[].projected.sources[].configMap.items[].*
spec.workspaces[].projected.sources[].secret.items[].*

Changes

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
pre-commit Passed
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
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

feat: support for variable interpolation in workspace.* (in PipelineRun and TaskRun)

/kind feature

tekton-robot · 2024-02-15T06:43:09Z

Hi @l-qing. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

l-qing · 2024-02-15T06:46:30Z

docs/variables.md

-| `Task`        | `spec.steps[].volumemounts.name`                                |
-| `Task`        | `spec.steps[].volumemounts.mountpath`                           |
-| `Task`        | `spec.steps[].volumemounts.subpath`                             |
+| `Task`        | `spec.steps[].env.valueFrom.secretKeyRef.name`                  |


It should actually be camel case notation.

This will go away once this is rebased on top of the related PR, right?

Yes, that's why I set up the do-not-merge/hold.

l-qing · 2024-02-15T06:47:03Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -4891,18 +4891,18 @@ spec:
      steps:
        - name: s1
          image: alpine
-          script: | 
+          script: |


Remove trailing whitespace.

l-qing · 2024-02-15T06:47:39Z

pkg/reconciler/pipelinerun/resources/apply.go

-			}
-		}
-	}
+	pr.Spec.Workspaces = workspace.ReplaceWorkspaceBindingsVars(pr.Spec.Workspaces, stringReplacements)


Refactor this section of the code so that it can be reused in taskrun and pipelinerun.

l-qing · 2024-02-15T06:48:14Z

pkg/workspace/apply.go

+
+// ReplaceWorkspaceBindingsVars returns a new slice of WorkspaceBinding with references to parameters replaced,
+// based on the mapping provided in replacements.
+func ReplaceWorkspaceBindingsVars(wbs []v1.WorkspaceBinding, replacements map[string]string) []v1.WorkspaceBinding {


This is an extracted public method for reuse in both taskrun and pipelinerun.

tekton-robot · 2024-02-15T06:52:03Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

tekton-robot · 2024-02-17T00:46:53Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

tekton-robot · 2024-02-17T01:12:48Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

tekton-robot · 2024-02-17T01:16: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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

tekton-robot · 2024-02-21T14:28:03Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

tekton-robot · 2024-02-22T10:41:28Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

l-qing · 2024-02-22T12:23:20Z

/cc @JeromeJu

JeromeJu · 2024-02-22T14:20:58Z

/ok-to-test

JeromeJu

Thanks @l-qing for this!
Would you mind splitting up the PR from refactoring and changing the variable camel cases according to https://github.com/tektoncd/community/blob/main/standards.md#small-pull-requests?

tekton-robot · 2024-02-22T14:29:29Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

tekton-robot · 2024-02-22T16:11:07Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

l-qing · 2024-02-22T16:13:32Z

/hold

Hold this PR until the #7701 is merged.

tekton-robot · 2024-02-22T16:14:32Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

l-qing · 2024-02-22T16:14:57Z

Thanks @l-qing for this! Would you mind splitting up the PR from refactoring and changing the variable camel cases according to https://github.com/tektoncd/community/blob/main/standards.md#small-pull-requests?

Thank you for your suggestion.
I've split the document modification PR #7701; please review it when you have time.

tekton-robot · 2024-02-26T13:54:04Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

afrittoli · 2024-02-26T13:58:40Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+			name: "projected.sources.secret.items success",
+			prs: []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, `
+metadata:
+  name: test-pipeline-run-different-service-accs


NIT: maybe the PR name should be more specific to the aspect we are testing here (variable substitution)

I found that the latter part of the unit tests fixed the name of the PR, so any modifications would have to be changed consistently.

pipeline/pkg/reconciler/pipelinerun/pipelinerun_test.go

Lines 6135 to 6140 in d27e6cf

_, clients := prt.reconcileRun("foo", "test-pipeline-run-different-service-accs", []string{}, false)

// Check that the expected TaskRun was created

actual, err := clients.Pipeline.TektonV1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{

LabelSelector: "tekton.dev/pipelineTask=b-task,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs",

Limit: 1,

})

I've uniformly renamed test-pipeline-run-different-service-accs to test-pipeline-run-variable-substitution to better reflect the purpose of this case.

tekton-robot · 2024-02-26T14:00:00Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

afrittoli

Thanks!
/approve

vdemeester

/lgtm

tekton-robot · 2024-02-26T16:05:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

…un and TaskRun) fix tektoncd#7667 Support for variable interpolation in: * spec.workspaces[].configMap.items[].* * spec.workspaces[].secret.items[].* * spec.workspaces[].projected.sources[].configMap.items[].* * spec.workspaces[].projected.sources[].secret.items[].*

l-qing · 2024-02-26T16:13:07Z

/unhold

vdemeester · 2024-02-26T16:15:05Z

/lgtm

l-qing · 2024-02-26T16:16:29Z

/lgtm

Thank you very much, I was just about to at you for comment. 😆

tekton-robot · 2024-02-26T16:19:46Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

tekton-robot · 2024-02-26T16:19:49Z

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/apply.go	98.7%	98.5%	-0.2
pkg/reconciler/taskrun/resources/apply.go	99.5%	99.5%	-0.0
pkg/workspace/apply.go	97.4%	98.1%	0.7

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 15, 2024

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 15, 2024

tekton-robot requested review from bobcatfish and vdemeester February 15, 2024 06:43

l-qing commented Feb 15, 2024

View reviewed changes

l-qing force-pushed the feat/populate-params-and-results-to-workspace-bindings branch from 21e1b05 to b6478cf Compare February 17, 2024 00:38

l-qing force-pushed the feat/populate-params-and-results-to-workspace-bindings branch 2 times, most recently from 7a8abcd to ca41dc9 Compare February 17, 2024 01:07

l-qing force-pushed the feat/populate-params-and-results-to-workspace-bindings branch from ca41dc9 to c7e512f Compare February 21, 2024 14:19

l-qing force-pushed the feat/populate-params-and-results-to-workspace-bindings branch from c7e512f to c61546f Compare February 22, 2024 10:33

tekton-robot requested a review from JeromeJu February 22, 2024 12:23

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 22, 2024

JeromeJu reviewed Feb 22, 2024

View reviewed changes

l-qing force-pushed the feat/populate-params-and-results-to-workspace-bindings branch from c61546f to 325e973 Compare February 22, 2024 16:05

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2024

l-qing force-pushed the feat/populate-params-and-results-to-workspace-bindings branch from 325e973 to 273bc7e Compare February 26, 2024 13:46

afrittoli reviewed Feb 26, 2024

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2024

vdemeester approved these changes Feb 26, 2024

View reviewed changes

tekton-robot assigned vdemeester Feb 26, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2024

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2024

l-qing force-pushed the feat/populate-params-and-results-to-workspace-bindings branch from 273bc7e to 42637e1 Compare February 26, 2024 16:12

tekton-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 26, 2024

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 26, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2024

tekton-robot merged commit 33365cf into tektoncd:main Feb 26, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for variable interpolation in workspace.* (in PipelineRun and TaskRun) #7671

feat: support for variable interpolation in workspace.* (in PipelineRun and TaskRun) #7671

l-qing commented Feb 15, 2024 •

edited

Loading

tekton-robot commented Feb 15, 2024

l-qing Feb 15, 2024

afrittoli Feb 26, 2024

l-qing Feb 26, 2024

l-qing Feb 15, 2024

l-qing Feb 15, 2024

l-qing Feb 15, 2024

tekton-robot commented Feb 15, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 21, 2024

tekton-robot commented Feb 22, 2024

l-qing commented Feb 22, 2024

JeromeJu commented Feb 22, 2024

JeromeJu left a comment

tekton-robot commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

l-qing commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

l-qing commented Feb 22, 2024

tekton-robot commented Feb 26, 2024

afrittoli Feb 26, 2024

l-qing Feb 26, 2024

tekton-robot commented Feb 26, 2024

afrittoli left a comment

vdemeester left a comment

tekton-robot commented Feb 26, 2024

l-qing commented Feb 26, 2024

vdemeester commented Feb 26, 2024

l-qing commented Feb 26, 2024

tekton-robot commented Feb 26, 2024

tekton-robot commented Feb 26, 2024

	_, clients := prt.reconcileRun("foo", "test-pipeline-run-different-service-accs", []string{}, false)
	// Check that the expected TaskRun was created
	actual, err := clients.Pipeline.TektonV1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{
	LabelSelector: "tekton.dev/pipelineTask=b-task,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs",
	Limit: 1,
	})

feat: support for variable interpolation in workspace.* (in PipelineRun and TaskRun) #7671

feat: support for variable interpolation in workspace.* (in PipelineRun and TaskRun) #7671

Conversation

l-qing commented Feb 15, 2024 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Feb 15, 2024

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 Feb 15, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 17, 2024

tekton-robot commented Feb 21, 2024

tekton-robot commented Feb 22, 2024

l-qing commented Feb 22, 2024

JeromeJu commented Feb 22, 2024

JeromeJu left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

l-qing commented Feb 22, 2024

tekton-robot commented Feb 22, 2024

l-qing commented Feb 22, 2024

tekton-robot commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Feb 26, 2024

afrittoli left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 26, 2024

l-qing commented Feb 26, 2024

vdemeester commented Feb 26, 2024

l-qing commented Feb 26, 2024

tekton-robot commented Feb 26, 2024

tekton-robot commented Feb 26, 2024

l-qing commented Feb 15, 2024 •

edited

Loading