-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change variable interpolation braces #1172
Change variable interpolation braces #1172
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish 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:
Approvers can indicate their approval by writing |
The following is the coverage report on pkg/.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $() syntax looks like bash variable replacement syntax. Folks actually using that syntax will need a way to escape it.
That was one of the cons. I thought it was a big one but actually,${…}
was one too, and is usually more used than$(…)
(that starts a subshell and execute the comand)… so I think we're all good on that.
I also do like substitution
more than interpolation
, it is indeed simpler 😉
steps: | ||
- name: read | ||
image: ubuntu | ||
command: ["/bin/bash"] | ||
args: ['-c', 'cat /workspace/newworkspace/stuff'] # tests that new targetpath and previous task output is dumped | ||
args: ['$(inputs.params.args)'] # tests that new targetpath and previous task output is dumped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice !
docs/resources.md
Outdated
@@ -419,6 +419,7 @@ spec: | |||
type: cluster | |||
params: | |||
- name: url | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: intentional extra line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops thanks :D
pkg/substitution/substitution.go
Outdated
@@ -76,7 +76,8 @@ func ValidateVariableIsolated(name, value, prefix, contextPrefix, locationName, | |||
// Extract a the first full string expressions found (e.g "${input.params.foo}"). Return | |||
// "" and false if nothing is found. | |||
func extractExpressionFromString(s, prefix string) (string, bool) { | |||
pattern := fmt.Sprintf("\\$({%s.(?P<var>%s)})", prefix, parameterSubstitution) | |||
//TODO(1170): Regex matches both ${} and $(), we will need to remove ${} compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most minor of nits: These todos are labelled differently from others elsewhere in the diff: TODO(1170)
vs TODO(#1170)
. The only reason this is noteworthy is because I assume we're going to want to grep for the pattern and remove all of them in future. Might be helpful to future us if all the todos are the same string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, thanks for catching this! :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think while im here i'm going to put that regex into a constant ... 😇
test/pipelinerun_test.go
Outdated
// Reference build: https:/knative/build/tree/master/test/docker-basic | ||
tb.Step("config-docker", "quay.io/rhpipeline/skopeo:alpine", | ||
tb.Command("skopeo"), | ||
tb.Args("copy", "docker://gcr.io/build-crd-testing/secret-sauce", "dir:///tmp/"), | ||
tb.Args("copy", "${inputs.params.path}", "${inputs.params.dest}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentionally using deprecated format here? If so, worth adding a TODO(#1170)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch - and actually I meant to mix the formatting but apparently forgot to 🤦♀ !
I added some v v small quibbles. Otherwise this looks good to me. |
They were excellent quibbles @sbwsg :D |
3022582
to
9fd9cec
Compare
Ready for another look! |
The following is the coverage report on pkg/.
|
/lgtm |
docs/tasks.md
Outdated
|
||
`Tasks` support templating using values from all [`inputs`](#inputs) and | ||
`Tasks` support string replcement using values from all [`inputs`](#inputs) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: replcement
-> replacement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!!
9fd9cec
to
7608519
Compare
The following is the coverage report on pkg/.
|
/lgtm |
Using the path replacement makes dealing with paths much easier - you don't need to know about `/workspace` or about when the path has been overridden or any of that. I'm also doing this to set the stage for tektoncd#850 - so that when we change from `${}` syntax to `$()` we can use this test to test for both types (to make sure the change is backward compatible).
In tektoncd#850 we want to make sure to support both $() and ${} syntax for all variable substitution, including pipeline params, so I've added these values to the test so we can update them to try both syntaxes.
When changing the ${} syntax to $() in tektoncd#850 I'd like to be able to test all reasonable instances of the syntax, of which the array type is one. Turns out we didn't have any examples or end to end tests using this syntax (until now!)
7608519
to
fcc2c3c
Compare
Rebased! |
The following is the coverage report on pkg/.
|
In tektoncd#850 we decided that to make our syntax more similar to k8s style syntax, we will use $() for variable substitution instead of ${}. In a later iteration we may also want to make it so that anything that can be acesssed as a variable is also available as an env var to the running container, but that is TBD. In the context of this discussion we also considered removing variable substitution entirely, that conversation is continuing. We have also tried to start calling this functionality "variable substitution" instead of "templating" to make it clear that we do not intend to support more complicated templating functionality. Co-authored-by: Eli Zucker <[email protected]>
In order to make it clear that we are not intending to support more complicated templating (via tektoncd#850), rename the "template" related logic to "substitution". @vdemeester suggested "variable interpolation" which seems to be completely correct but I am _hoping_ that "substitution" is a bit simpler than "interpolation" (might be less correct tho!) Fixes tektoncd#850
fcc2c3c
to
b2469f0
Compare
The following is the coverage report on pkg/.
|
/lgtm |
In tektoncd#1172 I tried to update these docs to use the new brace syntax but I only half updated this example :( And we have no tests for examples embedded in our docs so we can't catch this. Thanks for the bug report @moficodes !!!
In tektoncd#1172 I tried to update these docs to use the new brace syntax but I only half updated this example :( And we have no tests for examples embedded in our docs so we can't catch this. Thanks for the bug report @moficodes !!! Fixes tektoncd#1199
In #1172 I tried to update these docs to use the new brace syntax but I only half updated this example :( And we have no tests for examples embedded in our docs so we can't catch this. Thanks for the bug report @moficodes !!! Fixes #1199
In #1172 I tried to update these docs to use the new brace syntax but I only half updated this example :( And we have no tests for examples embedded in our docs so we can't catch this. Thanks for the bug report @moficodes !!! Fixes #1199 (cherry picked from commit 129a8ed)
In tektoncd#1199 @moficodes reported that the tutorial wasn't working which was b/c in tektoncd#1172 when I updated the braces, I only half updated the tutorial example. I wanted to fix this for the v0.6.0 docs as well, but since the released binary and yaml files don't need an update, it didn't make sense to me to create v0.6.1 for this (and leave the v0.6.0 docs broken!) so I created a 0.6 branch and cherry picked the docs fix into it only, and am now updating the README to point at that.
In #1199 @moficodes reported that the tutorial wasn't working which was b/c in #1172 when I updated the braces, I only half updated the tutorial example. I wanted to fix this for the v0.6.0 docs as well, but since the released binary and yaml files don't need an update, it didn't make sense to me to create v0.6.1 for this (and leave the v0.6.0 docs broken!) so I created a 0.6 branch and cherry picked the docs fix into it only, and am now updating the README to point at that.
In tektoncd#850 we decided that to make our syntax more similar to k8s style syntax, we will use $() for variable substitution instead of ${}. In a later iteration we may also want to make it so that anything that can be acesssed as a variable is also available as an env var to the running container, but that is TBD. In tektoncd#1172 we added support for the new syntax, $(), and continued support for ${}, which was released in 0.6. This commit removes support for ${}. Fixes tektoncd#1170
In #850 we decided that to make our syntax more similar to k8s style syntax, we will use $() for variable substitution instead of ${}. In a later iteration we may also want to make it so that anything that can be acesssed as a variable is also available as an env var to the running container, but that is TBD. In #1172 we added support for the new syntax, $(), and continued support for ${}, which was released in 0.6. This commit removes support for ${}. Fixes #1170
Changes
In #850 we decided that to make our syntax more similar to k8s style$() for variable substitution instead of $ {}. In a
syntax, we will use
later iteration we may also want to make it so that anything that can be
accesssed as a variable is also available as an env var to the running
container, but that is TBD.
In the context of this discussion we also considered removing variable
substitution entirely, that conversation is continuing.
We have also tried to start calling this functionality "variable
substitution" instead of "templating" to make it clear that we do not
intend to support more complicated templating functionality.
In order to make it clear that we are not intending to support more
complicated templating (via #850), rename the "template" related logic
to "substitution".
@vdemeester suggested "variable interpolation" which seems to be
completely correct but I am hoping that "substitution" is a bit
simpler than "interpolation" (might be less correct tho!)
Fixes #850
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmd
dir, please updatethe release Task and TaskRun to build and release this image
Release Notes