Implement implicit parameter resolution. #4127
Conversation
|
The following is the coverage report on the affected files.
|
|
/hold Looks like I missed some tests |
ab7c870 to
2edf64f
Compare
|
The following is the coverage report on the affected files.
|
|
/hold cancel |
|
/assign |
docs/pipelineruns.md
Outdated
| available to any inlined specs without needing to be explicitly defined. | ||
|
|
||
| ```yaml | ||
| apiVersion: tekton.dev/v1alpha1 |
There was a problem hiding this comment.
it looks like this functionality is being marked alpha by adding it only to the alpha type - thats a different approach than we've taken via TEP-0033 - the approach we're favoring is to us enable-api-fields: alpha to gate functionality like this - this is a controller setting though so if this is implemented in the webhook, maybe we need to either port that flag over to the webhook, or let the webhook read that value from the same configmap as the controller? probably doesnt make sense to need to configure it twice... TEP-0033 could use an update to include the webhook, though it does say:
Since the API fields will exist in the CRD with or without the flags, this will be enforced at runtime by the webhook admission controller: if the flag is not enabled and a user tries to use an alpha field, the webhook will not allow the CRD to be stored and will return an actionable error. (Same for beta.)
which feels like it implies that the webhook should be aware of this configuration
There was a problem hiding this comment.
hmmm - the TEP also says:
Use of alpha CRDs would not require enable-api-fields to be set, neither would beta, i.e. enable-api-fields only gates access to fields within CRDs, not to CRDs themselves.
It feels strange to require enabled-api-fields: alpha if you're already explicitly using the v1alpha1 API, though I think it does make sense to require this for any alpha fields in the v1beta that we can't easily separate out. I'm trying to avoid v1beta1 to avoid any deprecation policy concerns while we're trying this out. wdyt?
It makes sense to gate the loosened param constraints in the reconciler behind this flag though - I'll add this!
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-alpha-integration-tests pull-tekton-pipeline-integration-tests |
|
/hold Talked a bit off thread about this - seems like this should have been added to v1beta1 instead, relying on the alpha flag to enable the functionality. Will update. |
|
The following is the coverage report on the affected files.
|
bb79f3b to
a3d8230
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
c391337 to
c5ae153
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg 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 the affected files.
|
|
ping for review! |
|
thanks @wlynch for this, looking forward to this feature 😍 This will simplify the resource definitions a whole lot. I have started to review this PR. In meanwhile, can you please update the PR description and release notes to exclude |
4fd0098 to
ec97e05
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
I ran a params:
- name: MESSAGE
value: $(params.GREETINGS)Without these changes, the resolved spec:
pipelineSpec:
params:
- default: Hello World!
description: Greetings, default is Hello World!
name: GREETINGS
type: string
tasks:
- name: echo-greetings
params:
- name: MESSAGE
value: $(params.GREETINGS)
taskSpec:With these changes, in addition to the explicit mapping, the resolved spec includes an additional implicit mapping along with the spec:
pipelineSpec:
params:
- default: Hello World!
description: Greetings, default is Hello World!
name: GREETINGS
type: string
tasks:
- name: echo-greetings
params:
- name: MESSAGE
value: $(params.GREETINGS)
- name: GREETINGS
value: $(params.GREETINGS)
taskSpec:I am not sure if this can cause any conflicts but I think we can atleast document this behavior. Wdyt? I have the entire EDIT: This could potentially explode the list of parameters in each |
ec97e05 to
78969ca
Compare
|
The following is the coverage report on the affected files.
|
Copying my respond from Slack here (with minor edits) - I think this is WAI (at least for now). you would need it to cover weird edge cases like this: apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
generateName: echo-
spec:
params:
- name: MESSAGE1
value: "Good Morning!"
pipelineSpec:
params:
- name: MESSAGE2
default: Hello World!
tasks:
- name: echo-message
params:
- name: MESSAGE3
value: $(params.MESSAGE2)
taskSpec:
steps:
- name: echo
image: ubuntu
script: |
#!/usr/bin/env bash
echo "$(params.MESSAGE1)" "$(params.MESSAGE2)" "$(params.MESSAGE3)"This behavior seems to be most consistent with what we're trying to go for. e.g. if I was to write something similar in Go it would look like - a := "Good Morning!"
func() {
b := "Hello World!"
func(c string) {
fmt.Println(a, b, c)
}(b)
}()The params available always needs to be a union of the other parameters in the context, since we don't know which will be used. The verboseness of the output config is a consequence of us returning the fully resolved parameter context so it's clear which params reached the Task. It shouldn't cause conflicts - params defined lower in the stack should always take precedence, and unused parameters shouldn't modify behavior since they aren't used. I suppose there is a small concern around complex pipelines hitting etcd limits, but the work around there would be to stop inlining everything and break things into discrete Tasks. Alternatives I can think of:
The alternatives make me think a reasonable next step would be to look into accepting the implicit configs as-is, and see what impact this would have on the current parameter validation (and also get feedback if not including the fully resolved spec would make debugging harder). My preference would be to tackle this in another change/PR. Let me know what you think! |
This is the implementation of [TEP-0023](https://github.com/tektoncd/community/blob/main/teps/0023-implicit-mapping.md). This adds information to the defaulting context to allow parameters to be passed down the stack during evaluation. This functionality is gated behind the alpha feature flag. Additionally, Tasks are now allowed to accept more params than are actually used. This should generally be safe, since extra params that are provided should not affect the behavior of the Task itself.
78969ca to
eeebe24
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
| Note that all implicit Parameters will be passed through to inlined resources | ||
| (i.e. PipelineRun -> Pipeline -> Tasks) even if they are not used. | ||
| Extra parameters passed this way should generally be safe (since they aren't | ||
| actually used), but may result in more verbose specs being returned by the API. |
There was a problem hiding this comment.
thanks a bunch @wlynch for adding this note 🙏
|
thanks a bunch @wlynch
Yup, we can certainly tackle this in a follow up PR. My major concern is exploding the size of the /lgtm 🤞 for the integration tests |
|
/test pull-tekton-pipeline-integration-tests |
|
/test pull-tekton-pipeline-alpha-integration-tests |
Changes
This is the implementation of
TEP-0023.
This adds information to the defaulting context to allow parameters to
be passed down the stack during evaluation. This functionality is gated
behind the alpha feature flag.
Additionally, Tasks are now allowed to accept more params than are
actually used. This should generally be safe, since extra params that
are provided should not affect the behavior of the Task itself.
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes