-
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] Add enum API field #7289
[TEP-0144] Add enum API field #7289
Conversation
Skipping CI for Draft Pull Request. |
/test all |
/hold until #7279 is merged. |
22ef994
to
3e6b9e3
Compare
/test all |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
3e6b9e3
to
8c15157
Compare
/test pull-tekton-pipeline-build-tests |
8c15157
to
c68093e
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
c68093e
to
40dcb05
Compare
The following is the coverage report on the affected files.
|
40dcb05
to
484e4ea
Compare
/hold cancel |
The following is the coverage report on the affected files.
|
/assign |
pkg/apis/pipeline/v1/param_types.go
Outdated
continue | ||
} | ||
if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { | ||
errs = errs.Also(errs, apis.ErrGeneric(fmt.Sprintf("feature flag `%s` should be set to true to use Enum", config.EnableParamEnum), "").ViaFieldKey("params", p.Name)) |
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.
Instead of adding ViaFieldKey
("params", ...) for every check here, why not just do
ViaFieldKey(p.Name)
and where you call validateParamEnums
call
errs = errs.Also(params.validateParamEnums(ctx).ViaField("params"))
?
func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { | ||
// validates all the types within a slice of ParamSpecs | ||
errs = errs.Also(ValidateParameterTypes(ctx, params).ViaField("params")) | ||
errs = errs.Also(params.validateNoDuplicateNames()) | ||
errs = errs.Also(params.validateParamEnums(ctx)) |
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.
errs = errs.Also(params.validateParamEnums(ctx).ViaField("params"))? And in the checks there, call ViaFieldKey(p.Name)
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! Updated (and for all comments below), PTAL!
@@ -435,6 +435,7 @@ func (p ParamSpec) ValidateObjectType(ctx context.Context) *apis.FieldError { | |||
func ValidateParameterVariables(ctx context.Context, steps []Step, params ParamSpecs) *apis.FieldError { | |||
var errs *apis.FieldError | |||
errs = errs.Also(params.validateNoDuplicateNames()) | |||
errs = errs.Also(params.validateParamEnums(ctx)) |
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.
same comment as above related to ViaField
.
} | ||
if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum { | ||
errs = errs.Also(errs, apis.ErrGeneric(fmt.Sprintf("feature flag `%s` should be set to true to use Enum", config.EnableParamEnum), "").ViaFieldKey("params", p.Name)) | ||
} |
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.
Same comment related to ViaFieldKey
func ValidatePipelineParameterVariables(ctx context.Context, tasks []PipelineTask, params ParamSpecs) (errs *apis.FieldError) { | ||
// validates all the types within a slice of ParamSpecs | ||
errs = errs.Also(ValidateParameterTypes(ctx, params).ViaField("params")) | ||
errs = errs.Also(params.validateNoDuplicateNames()) | ||
errs = errs.Also(params.validateParamEnums(ctx)) |
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.
Same comment related to ViaField
@@ -441,6 +441,7 @@ func (p ParamSpec) ValidateObjectType(ctx context.Context) *apis.FieldError { | |||
func ValidateParameterVariables(ctx context.Context, steps []Step, params ParamSpecs) *apis.FieldError { | |||
var errs *apis.FieldError | |||
errs = errs.Also(params.validateNoDuplicateNames()) | |||
errs = errs.Also(params.validateParamEnums(ctx)) |
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.
Same comment related to ViaField
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.
Left some comments. Otherwise lgtm!
56018ca
to
c25d02a
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
Thanks @Yongxuanzhang! Could you please take a look when you get a chance 🙏 ? |
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.
looks the test cases are duplicate in pipeline and task, could be a followup cleanup to merge the tests? I'm not sure 😄
@@ -2254,3 +2254,86 @@ func TestGetArrayIndexParamRefs(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func TestParamEnum(t *testing.T) { |
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.
better separate this to success and failure like tests in pipeline validation
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've splitted the tests, PTAL!
[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 |
Yeah I see the confusion. I think it "looks like a duplicate" is because |
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 the `Enum` api field, validation and conversion logic. /kind feature [tektoncd#7270]: tektoncd#7270 [tep-0144]: https://github.com/tektoncd/community/blob/main/teps/0144-param-enum.md
c25d02a
to
9eadaf3
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/retest |
Thanks @QuanZhang-William |
Changes
Part of #7270. In TEP-0144 we proposed a new
enum
field to support built-in param input validation.This commit adds the
Enum
api field, validation and conversion logic./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