-
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
[TEP-0144] Validate PipelineRun for Param Enum #7338
[TEP-0144] Validate PipelineRun for Param Enum #7338
Conversation
Skipping CI for Draft Pull Request. |
/test all |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
879ee96
to
e8cac6e
Compare
/retest |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
e8cac6e
to
344c756
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
344c756
to
cd1b509
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
cd1b509
to
c113908
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/retest |
1874f92
to
28ae20e
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Yongxuanzhang 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 |
@tektoncd/core-collaborators @tektoncd/core-maintainers. Please take a look if you are interested 😃 |
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 have a concern about the intersection approach when enum
is specified in both tasks and pipelines, I think we should require that the enum
in pipeline is a subset of the enum
in task
Also, what happens currently when the param in pipeline is passed to multiple tasks with enums? do you take the intersection of the enums in all the tasks the param is passed to and the pipeline? in the example below, what is the valid value for "message" param in a pipelinerun?
apiVersion: tekton.dev/v1
kind: Task
metadata:
name: param-enum-demo-1
spec:
params:
- name: message
type: string
enum: ["v1", "v2", "v3"]
steps:
- name: build
image: bash:latest
script: |
echo "$(params.message)"
---
apiVersion: tekton.dev/v1
kind: Task
metadata:
name: param-enum-demo-2
spec:
params:
- name: message
type: string
enum: ["v3", "v4"]
steps:
- name: build
image: bash:latest
script: |
echo "$(params.message)"
---
apiVersion: tekton.dev/v1
kind: Pipeline
metadata:
name: pipeline-param-enum
spec:
params:
- name: message
enum: ["v1", "v3", "v4", "v5"]
tasks:
- name: task-1
params:
- name: message
value: $(params.message)
taskRef:
name: param-enum-demo-1
- name: task-2
params:
- name: message
value: $(params.message)
taskRef:
name: param-enum-demo-2
/hold wait on tektoncd/community#1106 to be merged |
28ae20e
to
08d7855
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
Hi Jerop. You are right about the behaviour in the previous intersection design, it essentially takes the intersection of all related enum sets. I have changed to subset design, which also essentially validates the pipeline-level is a subset of all referenced Tasks |
08d7855
to
8de76a8
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
Part of [tektoncd#7270][tektoncd#7270]. In [TEP-0144][tep-0144] we proposed a new `enum` field to support built-in param input validation. This commit adds validation logic for PipelineRun against Param Enum /kind feature [tektoncd#7270]: tektoncd#7270 [tep-0144]: https://github.com/tektoncd/community/blob/main/teps/0144-param-enum.md
8de76a8
to
67a34d9
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
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.
Thank you @QuanZhang-William!
/lgtm
Changes
Part of #7270. In TEP-0144 we proposed a new
enum
field to support built-in param input validation.This commit adds validation logic for PipelineRun against Param Enum
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes