Document all locations that variable substitution is available #2927

psschwei · 2020-07-10T01:02:04Z

Changes

Fixes #2605 , adding a table to docs/variables.md with locations in our CRDs where variables will be successfully interpolated into.

Submitter Checklist

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

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.

psschwei · 2020-07-10T01:02:24Z

/kind documentation

psschwei · 2020-07-10T01:03:58Z

/release-note-none

pritidesai · 2020-07-10T06:09:45Z

@psschwei please squash three commits into one 🙏

pritidesai · 2020-07-10T06:31:45Z

docs/variables.md

+| --- | ----- |
+| `Task` | `credentials.path` | 
+| `Task` | `spec.steps[].name` |
+| `Task` | `spec.steps[].image` |


this is great 👍

pritidesai · 2020-07-10T06:33:20Z

docs/variables.md

+| `Task` | `spec.steps[].env.valuefrom.secretkeyref.name` |
+| `Task` | `spec.steps[].env.valuefrom.secretkeyref.value` |
+| `Task` | `spec.steps[].env.valuefrom.configmapkeyref.name` |
+| `Task` | `spec.steps[].env.valuefrom.configmapkeyref.value` |


I think these five can be combined into one single spec.steps[].env, @sbwsg thoughts?

My one concern is that we don't provide substitution for all fields under env - for example we don't substitute the .env.name field, nor any fields under .env.fieldRef or .env.resourceFieldRef.

My feeling is we should be more specific rather than less due to that. wdyt @pritidesai ?

afrittoli · 2020-07-10T11:23:36Z

/test pull-tekton-pipeline-unit-tests

ghost · 2020-07-10T13:26:18Z

docs/variables.md

@@ -113,3 +113,36 @@ variable via `resources.inputs.<resourceName>.<variableName>` or
 | `name` | The name of the resource. |
 | `type` | Type value of `"cloudEvent"`. |
 | `target-uri` | The URI to hit with cloud event payloads. |
+
+## Variables that can be substituted in a CRD


Suggest "Task fields that accept variable substitutions" for the title here.

ghost · 2020-07-10T13:37:42Z

docs/variables.md

+| `Task` | `spec.steps[].image` |
+| `Task` | `spec.steps[].env.value` |
+| `Task` | `spec.steps[].env.valuefrom.secretkeyref.name` |
+| `Task` | `spec.steps[].env.valuefrom.secretkeyref.value` |


I think this should .key instead of .value? Similarly for configmapkeyref.

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#configmapkeyselector-v1-core

https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/v1beta1/container_replacements.go#L40

ghost · 2020-07-10T13:41:18Z

docs/variables.md

+| `Task` | `spec.steps[].env.valuefrom.secretkeyref.name` |
+| `Task` | `spec.steps[].env.valuefrom.secretkeyref.value` |
+| `Task` | `spec.steps[].env.valuefrom.configmapkeyref.name` |
+| `Task` | `spec.steps[].env.valuefrom.configmapkeyref.value` |


My one concern is that we don't provide substitution for all fields under env - for example we don't substitute the .env.name field, nor any fields under .env.fieldRef or .env.resourceFieldRef.

My feeling is we should be more specific rather than less due to that. wdyt @pritidesai ?

ghost · 2020-07-10T14:28:27Z

docs/variables.md

+| `Task` | `spec.steps[].env.valuefrom.configmapkeyref.value` |
+| `Task` | `spec.steps[].volumemounts.name` |
+| `Task` | `spec.steps[].volumemounts.mountpath` |
+| `Task` | `spec.steps[].volumemounts.submountpath` |


I think this should be subpath?

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#volumemount-v1-core

https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/v1beta1/container_replacements.go#L70

ghost · 2020-07-10T14:34:30Z

docs/variables.md

+| `Task` | `spec.volumes[].persistentvolumeclaim.claimname` |
+| `Task` | `spec.volumes[].projected.sources.configmap.name` |
+| `Task` | `spec.volumes[].projected.sources.secret.name` |
+| `Task` | `spec.volumes[].projected.sources.serviceaccounttoken.audience` |


These were also very recently added:

spec.volumes[].csi.nodepublishsecretref.name spec.volumes[].csi.volumeattributes.*

That second one is a map of unspecified string keys so I'm not sure what the best way is to express it other than to use an *.

ghost · 2020-07-10T14:35:15Z

docs/variables.md

+| `Task` | `spec.sidecars[].image` |
+| `Task` | `spec.sidecars[].env.value` |
+| `Task` | `spec.sidecars[].env.valuefrom.secretkeyref.name` |
+| `Task` | `spec.sidecars[].env.valuefrom.secretkeyref.value` |


same comment as above wrt .key vs .value for secrets and configmaps.

ghost · 2020-07-10T14:35:42Z

docs/variables.md

+| `Task` | `spec.sidecars[].env.valuefrom.configmapkeyref.value` |
+| `Task` | `spec.sidecars[].volumemounts.name` |
+| `Task` | `spec.sidecars[].volumemounts.mountpath` |
+| `Task` | `spec.sidecars[].volumemounts.submountpath` |


and here submountpath -> subpath I think.

ghost · 2020-07-14T16:53:25Z

docs/variables.md

+
+| CRD | Field |
+| --- | ----- |
+| `Task` | `credentials.path` | 


This one is in error I think. It isn't a field that can receive variables.

ghost

This is a very useful improvement, thank you! I think this just needs to have commits squashed into 1 and then I'll approve.

ghost · 2020-07-14T17:01:30Z

Oh, also, do you plan to include Pipeline locations in this PR as well? Or prefer to merge as-is and tackle Pipeline locations separately?

psschwei · 2020-07-14T17:34:13Z

This is a very useful improvement, thank you! I think this just needs to have commits squashed into 1 and then I'll approve.

Thanks for the great review! Made it really easy for me. :)

Oh, also, do you plan to include Pipeline locations in this PR as well? Or prefer to merge as-is and tackle Pipeline locations separately?

I was planning to do them both in the same PR, but I can split them up if that's easier. Let me know which you'd prefer.

ghost · 2020-07-14T17:37:19Z

I was planning to do them both in the same PR, but I can split them up if that's easier. Let me know which you'd prefer.

Together in the same PR would be great.

psschwei · 2020-07-14T18:27:54Z

Together in the same PR would be great.

Will get the pipeline subs in later this week. Sorry for the delay in completing this!

psschwei · 2020-07-21T02:38:17Z

/assign @pritidesai

tekton-robot · 2020-07-21T12:15:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

ghost · 2020-07-21T12:31:03Z

docs/variables.md

+| `Task` | `spec.sidecars[].volumemounts.mountpath` |
+| `Task` | `spec.sidecars[].volumemounts.subpath` |
+| `Pipeline` | `spec.tasks[].params[].value` |
+| `Pipeline` | `spec.tasks[].conditions[].params[].value` |


One more to add here: Pipeline | spec.results[].value

adding variable substitutions for pipelines

ghost · 2020-07-21T15:13:43Z

/lgtm

Fantastic, thanks for taking this on.

tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jul 10, 2020

tekton-robot requested review from afrittoli and bobcatfish July 10, 2020 01:02

psschwei marked this pull request as draft July 10, 2020 01:02

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 10, 2020

tekton-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Jul 10, 2020

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 10, 2020

pritidesai reviewed Jul 10, 2020

View reviewed changes

afrittoli added kind/documentation Categorizes issue or PR as related to documentation. and removed kind/documentation Categorizes issue or PR as related to documentation. labels Jul 10, 2020

ghost reviewed Jul 10, 2020

View reviewed changes

psschwei force-pushed the docs-variable-substitution branch from 0a1af4d to 8d26e01 Compare July 14, 2020 16:51

ghost reviewed Jul 14, 2020

View reviewed changes

psschwei force-pushed the docs-variable-substitution branch from 08fde5c to c441126 Compare July 14, 2020 18:25

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 Jul 21, 2020

psschwei force-pushed the docs-variable-substitution branch from a76cd16 to e4a9cbf Compare July 21, 2020 01:55

psschwei marked this pull request as ready for review July 21, 2020 01:56

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 Jul 21, 2020

tekton-robot assigned pritidesai Jul 21, 2020

vdemeester approved these changes Jul 21, 2020

View reviewed changes

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

ghost reviewed Jul 21, 2020

View reviewed changes

adding table with variables that can be substituted in a CRD

4b63f1d

adding variable substitutions for pipelines

psschwei force-pushed the docs-variable-substitution branch from 2c6cc01 to 4b63f1d Compare July 21, 2020 15:06

tekton-robot assigned ghost Jul 21, 2020

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

tekton-robot merged commit 560dea2 into tektoncd:master Jul 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document all locations that variable substitution is available #2927

Document all locations that variable substitution is available #2927

psschwei commented Jul 10, 2020 •

edited

Loading

psschwei commented Jul 10, 2020

psschwei commented Jul 10, 2020

pritidesai commented Jul 10, 2020

pritidesai Jul 10, 2020

pritidesai Jul 10, 2020

ghost Jul 10, 2020

afrittoli commented Jul 10, 2020

ghost Jul 10, 2020

ghost Jul 10, 2020

ghost Jul 10, 2020

ghost Jul 10, 2020

ghost Jul 10, 2020

ghost Jul 10, 2020

ghost Jul 10, 2020

ghost Jul 14, 2020

ghost left a comment

ghost commented Jul 14, 2020

psschwei commented Jul 14, 2020

ghost commented Jul 14, 2020

psschwei commented Jul 14, 2020

psschwei commented Jul 21, 2020

tekton-robot commented Jul 21, 2020

ghost Jul 21, 2020

ghost commented Jul 21, 2020

Document all locations that variable substitution is available #2927

Document all locations that variable substitution is available #2927

Conversation

psschwei commented Jul 10, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

psschwei commented Jul 10, 2020

psschwei commented Jul 10, 2020

pritidesai commented Jul 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli commented Jul 10, 2020

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

ghost left a comment

Choose a reason for hiding this comment

ghost commented Jul 14, 2020

psschwei commented Jul 14, 2020

ghost commented Jul 14, 2020

psschwei commented Jul 14, 2020

psschwei commented Jul 21, 2020

tekton-robot commented Jul 21, 2020

Choose a reason for hiding this comment

ghost commented Jul 21, 2020

psschwei commented Jul 10, 2020 •

edited

Loading