Expand parameters to support the Array type. #1080
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
size/XXL
EliZucker
force-pushed
the
newarrayparams
branch
2 times, most recently
from
801ddf8 to
85037a0
Compare
|
/retest |
|
This may need corollary work in the CLI tool, tkn, or perhaps just a new version to be cut with this updated code? I just installed your controller and applied your example YAML, then ran ~/dev/go/src/github.com/tektoncd/manual-tests/array-or-string [12/118]
λ tkn pipelinerun list
Failed to list pipelineruns from default namespace
Error: v1alpha1.PipelineRunList.Items: []v1alpha1.PipelineRun: v1alpha1.PipelineRun.Spec: v1alpha1.PipelineRunSpec.Params: []v1alpha1.Param: v1alpha1.Param.Value: R
eadString: expects " or n, but found [, error found in #10 byte of ...|,"value":["element1"|..., bigger context ...|da"},"spec":{"params":[{"name":"context","value$
:["element1","element2"]}],"pipelineRef":{"name":"pi|...
Usage:
tkn pipelinerun list [flags]So I think whatever tkn is using to unmarshall the JSON response from the kube api needs to be updated to take your array support into account. Edit: Also, note, this issue is not a blocker for this PR. Just something to keep an eye one when this feature gets merged. |
Indeed, I'll create an issue in the cli 👼 |
|
/assign @imjasonh |
EliZucker
force-pushed
the
newarrayparams
branch
from
ee4d503 to
de4d811
Compare
EliZucker
force-pushed
the
newarrayparams
branch
from
de4d811 to
d7a358d
Compare
|
The following is the coverage report on pkg/.
|
EliZucker
force-pushed
the
newarrayparams
branch
from
d7a358d to
ed1686e
Compare
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
EliZucker
force-pushed
the
newarrayparams
branch
from
1a9b26f to
50264b3
Compare
|
The following is the coverage report on pkg/.
|
EliZucker
force-pushed
the
newarrayparams
branch
from
50264b3 to
fee9171
Compare
|
The following is the coverage report on pkg/.
|
|
The following is the coverage report on pkg/.
|
|
Tagging @dlorenc because I heard you set up the code coverage reports: I'm not sure if it's something I'm doing wrong from my end, but the coverage reports clearly contradict what IntelliJ is reporting (and I think IntelliJ is right?). Specifically templating.go is reported for having much lower coverage than intellij reports (see my comparison here). |
EliZucker
force-pushed
the
newarrayparams
branch
from
58e3c39 to
3127351
Compare
EliZucker
force-pushed
the
newarrayparams
branch
from
8bdf40b to
b84e333
Compare
|
The following is the coverage report on pkg/.
|
| @@ -118,16 +118,25 @@ func TestPipelineSpec_Validate(t *testing.T) { | |||
| }, { | |||
| name: "valid parameter variables", | |||
| p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( | |||
| tb.PipelineParam("baz"), | |||
| tb.PipelineParam("foo-is-baz"), | |||
| tb.PipelineParamSpec("baz", v1alpha1.ParamTypeString), | |||
There was a problem hiding this comment.
@imjasonh Just renamed this from PipelineParam to PipelineParamSpec to match changes in #1037 (I forgot to rename the builder functions).
Thinking more about trying to make the type optional here (disregarding complexity of implementing an optional parameter), we encourage users to explicitly mark their parameter types in ParamSpec but default to string partially for backwards compatibility. Is it that bad to have the types explicit for this builder function? We also don't have to worry about backwards compatibility here.
Edit: Also should mention that PipelineResources also follow a similar builder pattern: func PipelineResourceSpec(resourceType v1alpha1.PipelineResourceType, ops ...PipelineResourceSpecOp)
| @@ -227,6 +231,21 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er | |||
| return nil | |||
| } | |||
|
|
|||
| // Ensure that the parameters from the PipelineRun are overriding Pipeline parameters with the same type. | |||
There was a problem hiding this comment.
Oh interesting! In my head, I see webhook validation as validation that only uses information on that isolated YAML file being submitted (e.g invalid types, invalid variable interpolation strings), while reconciler validation brings in outside resources and evaluates the collective context.
In this case, since the validation requires finding the referenced pipeline resource, I put it in the reconciler -- this particular test verifies that, for example, a pipelinerun parameter string value is not overriding a corresponding pipeline's parameter array value. This seems really similar to the validation done with GetResourcesFromBindings (will validate that all PipelineResources declared in Pipeline p are bound in PipelineRun pr), which is also in the reconciler.
Writing this up, I think my decision makes sense, but let me know if it doesn't!
|
/lgtm |
tekton-robot
added
lgtm
|
/retest |
Instead of just strings, add support for users to supply parameters in the form of an array. Also include appropriate type validation and defaulting to support backwards-compatibility. Fixes tektoncd#207. The size of a user-supplied array is not specified anywhere in the corresponding ParamSpec, which is useful for defining tasks that require a dynamic number of strings used as distinct elements in an array. For instance, one might use an array parameter in an image-building task that feeds in a dynamic number of flags via the args field for a particular step. The implementation is defined such that an Array parameter can only be used if the replacement string is completely isolated and within a field that is an array of strings (currently eligible fields are 'command' and 'args'). For instance, the webhook will prevent an array parameter from being used in the 'image' field. Similarly, an array parameter used in a composite string, like 'value=${array}', is invalid and will be caught. The decision was made to completely prevent any use of array parameters as strings because it clutters Tekton with unnecessary functionality and is naturally extensible beyond the intended scope. For instance, if the behavior was defined such that array parameters used in the context of strings were automatically converted by separating them with a space, then that relatively arbitrary implementation could be questioned and eventually lead to needing to specify a custom delimiter (like commas), and so on. Another implementation detail to note is that the ArrayOrString type was necessary, in combination with implementing json.Marshaller, to support backwards-compatibility with strings. This is based off IntOrString from kubernetes/apimachinery, and allows the value-type to be immediately parsed and decided through the webhook. Lastly, a possibly unintuitive design decision is that if an EMPTY array parameter properly replaces an array element, then that string in the original array will be completely removed.
Following feedback from @imjasonh, rename ArrayOrStringParam to Param and rename Param to ResourceParam. This makes sense becase ArrayOrStringParam represented a more versatile type, while Param referred to a specific string-only type exclusively used for PipelineResources (like a git URL).
Change conditional tests to reflect the new ArrayOrString type. Also condense some builders that repeat the same ParamSpec building logic; the same ParamSpec building methods were repeated for Tasks, Pipelines, and Conditions, just with different names.
EliZucker
force-pushed
the
newarrayparams
branch
from
b84e333 to
d2c009d
Compare
|
The following is the coverage report on pkg/.
|
|
@imjasonh @afrittoli If one of you could give an lgtm that would be great! I had to rebase/refactor a bit to merge correctly with the initial conditional stuff, so the label was removed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli, dibyom, EliZucker, ImJasonH 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 |
|
🎉 Niiiiiiiiiice |
@vdemeester pointed out that tektoncd#1080 fixes a previous issue where default empty string parameters weren't working properly. Add a test to ensure this new functionality remains.
@vdemeester pointed out that #1080 fixes a previous issue where default empty string parameters weren't working properly. Add a test to ensure this new functionality remains.
Changes
Introduction:
This change was a lot harder than I initially thought it would be! There were a lot of unexpected design considerations and edge cases to catch.
I'm not sure how to logically divide this PR, since all of the test refactoring depends on the new ArrayOrString type so can't be in a separate commit.
Please let me know if you have any suggestions, since this is my first PR on Tekton of this size!
This PR is technically incomplete because I want to make the user documentation as clear as possible. However, the actual code for funtionality/tests is in a separate commit and ready for review.PR is completely ready for review. Thanks!Commit message:
Expand parameters to support the Array type.
Instead of just strings, add support for users to supply parameters in the form of an array. Also include appropriate type validation and defaulting to support backwards-compatibility. Fixes #207.
The size of a user-supplied array is not specified anywhere in the corresponding
ParamSpec, which is useful for defining tasks that require a dynamic number of strings used as distinct elements in an array. For instance, one might use an array parameter in an image-building task that feeds in a dynamic number of flags via theargsfield for a particular step.The implementation is defined such that an Array parameter can only be used if the replacement string is completely isolated and within a field that is an array of strings (currently eligible fields are
commandandargs). For instance, the webhook will prevent an array parameter from being used in theimagefield. Similarly, an array parameter used in a composite string, like"value=${array}", is invalid and will be caught. The decision was made to completely prevent any use of array parameters as strings because it clutters Tekton with unnecessary functionality and is naturally extensible beyond the intended scope. For instance, if the behavior was defined such that array parameters used in the context of strings were automatically converted by separating them with a space, then that relatively arbitrary implementation could be questioned and eventually lead to needing to specify a custom delimiter (like commas), and so on.Another implementation detail to note is that the
ArrayOrStringtype was necessary, in combination with implementing json.Marshaller, to support backwards-compatibility with strings. This is based offIntOrStringfrom kubernetes/apimachinery, and allows the value-type to be immediately parsed and decided through the webhook.Lastly, a possibly unintuitive design decision is that if an EMPTY array parameter properly replaces an array element, then that string in the original array will be completely removed.
Example YAML (not in commit message):
Feel free to deploy pipelines with this PR's code changes and try out array parameters for yourself! Here is an example YAML that should run:
After running this, the step's pod reads the following args:
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:
cmddir, please updatethe release Task and TaskRun to build and release this image
Release Notes