Enhance v1beta1 validation code for task 🍸 #3185

vdemeester · 2020-09-09T15:32:15Z

Changes

Opening this to gather some feedback.

This make a better use of knative.dev/pkg apis errors package in order
to make the code simpler, and the report better.

This allows reporting multiple validation error at once — so, one
failure message (webhook error) would be more useful to the user.
This enhance the report as it displays more information, like
resources.inputs[0].invalidsource.type instead of
resources.inputs.source.type (the index appear, it's easier for the
user to know where the error is).
This reduce if err != nil path, simplifying the code.

/hold

This needs more work (at least on this package)
~~Tests that depends on some of this may fail, I need to fix it~~
It's gonna be really useful on function like validateVariables — it's now used there 😉
This PR would only do the Task part (without moving too much code away), follow-up includes
- applying this to the rest of the v1beta1 package
- extract some code into more files (those are big files, lot's of stuff could be extracted) and more functions
- make a pass on what is exported, and why

/cc @afrittoli @bobcatfish @sbwsg @nikhil-thomas

Signed-off-by: Vincent Demeester [email protected]

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Task validation now reports all the error at once, and is a bit more detailed.

vdemeester · 2020-09-09T15:32:27Z

/hold

vdemeester · 2020-09-09T15:32:33Z

/kind cleanup

pkg/apis/pipeline/v1beta1/task_validation.go

tekton-robot · 2020-09-09T15:36:33Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	85.3%	0.6

tekton-robot · 2020-09-10T09:59:16Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	85.3%	0.6

vdemeester · 2020-09-10T10:50:12Z

pkg/substitution/substitution.go

@@ -44,6 +44,22 @@ func ValidateVariable(name, value, prefix, locationName, path string, vars sets.
 	return nil
 }

+func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError {


This is a temporary simplified function that would replace ValidateVariable. Didn't change ValidateVariable because it is used elswhere 🙃

tekton-robot · 2020-09-10T10:52:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	89.8%	5.1
pkg/substitution/substitution.go	60.4%	54.2%	-6.1

vdemeester · 2020-09-10T11:07:39Z

pkg/apis/pipeline/v1beta1/task_validation.go

+	return errs
+}
+
+func (p ParamSpec) ValidateType() *apis.FieldError {


This should move on a different _validation.go file 😉

yup would be great to move such functions 🙏

tekton-robot · 2020-09-10T11:10:03Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	94.2%	9.6
pkg/substitution/substitution.go	60.4%	43.8%	-16.5

vdemeester · 2020-09-10T11:26:00Z

pkg/apis/pipeline/v1beta1/task_validation.go


+func (tr TaskResult) Validate(_ context.Context) *apis.FieldError {


This might also go in a separate _validation.go file

tekton-robot · 2020-09-10T11:28:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	94.3%	9.7
pkg/substitution/substitution.go	60.4%	43.8%	-16.5

tekton-robot · 2020-09-10T11:33:11Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	94.3%	9.7
pkg/substitution/substitution.go	60.4%	43.8%	-16.5

vdemeester · 2020-09-10T11:40:41Z

Should be good to go for task_validation.go mostly. To see the "end" result, https://github.com/tektoncd/pipeline/blob/cfec788f7f4f562b8906630fe7316ae189b66047/pkg/apis/pipeline/v1beta1/task_validation.go

tekton-robot · 2020-09-10T11:42:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	94.3%	9.7
pkg/substitution/substitution.go	60.4%	43.8%	-16.5

tekton-robot · 2020-09-10T11:51:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	94.3%	9.7
pkg/substitution/substitution.go	60.4%	43.8%	-16.5

tekton-robot · 2020-09-10T12:46:45Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	95.0%	10.3
pkg/substitution/substitution.go	60.4%	43.8%	-16.5

vdemeester · 2020-09-14T10:41:32Z

/hold cancel

pritidesai · 2020-09-15T18:31:08Z

pkg/apis/pipeline/v1beta1/resource_types_validation.go

-	}
-	if err := validateTaskResources(tr.Outputs, "outputs"); err != nil {
-		return err
+func (tr *TaskResources) Validate(ctx context.Context) (errs *apis.FieldError) {


pointer named return value 😞, could this result in reference to nil on line 30 or 31?

yes and no, that's the beauty of it 😛 (and this part of knative.dev/pkg)
See https://pkg.go.dev/knative.dev/[email protected]/apis#FieldError.Also (implementation is here).

func (fe *FieldError) isEmpty() bool { if fe == nil { return true } return fe.Message == "" && fe.Details == "" && len(fe.errors) == 0 && len(fe.Paths) == 0 }

It won't ever panic, because it knows how to act on a nil struct 😉

pritidesai

Huge cleanup and improvement, error message are more meaningful with indices 🙏

tekton-robot · 2020-09-18T23:26:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai

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:

~~OWNERS~~ [pritidesai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pritidesai · 2020-09-18T23:31:08Z

pkg/apis/pipeline/v1beta1/task_validation.go

+	errs = errs.Also(validateTaskVariable(step.WorkingDir, prefix, vars).ViaField("workingDir"))
+	errs = errs.Also(validateTaskVariable(step.Script, prefix, vars).ViaField("script"))
+	for i, cmd := range step.Command {
+		errs = errs.Also(validateTaskVariable(cmd, prefix, vars).ViaFieldIndex("command", i))


NIT: could have been into a common function (line 296 - 308) which is also appearing above

savitaashture

/lgtm

This make a better use of knative.dev/pkg apis errors package in order to make the code simpler, *and* the report better. - This allows reporting multiple validation error at once — so, one failure message (webhook error) would be more useful to the user. - This enhance the report as it displays more information, like `resources.inputs[0].invalidsource.type` instead of `resources.inputs.source.type` (the index appear, it's easier for the user to know where the error is). - This reduce `if err != nil` path, simplifying the code. This only tackle this for task_validation, and should be followed-up with other refactoring on the different `*_validation.go` files. Signed-off-by: Vincent Demeester <[email protected]>

tekton-robot · 2020-09-21T08:40:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	84.7%	95.0%	10.3
pkg/substitution/substitution.go	60.4%	43.8%	-16.5

savitaashture · 2020-09-21T09:15:18Z

/test pull-tekton-pipeline-integration-tests

ghost · 2020-09-21T12:03:06Z

/test pull-tekton-pipeline-integration-tests

savitaashture · 2020-09-21T13:39:25Z

/lgtm

tekton-robot requested review from afrittoli, bobcatfish, nikhil-thomas and a user September 9, 2020 15:32

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 9, 2020

vdemeester commented Sep 9, 2020

View reviewed changes

pkg/apis/pipeline/v1beta1/task_validation.go Outdated Show resolved Hide resolved

vdemeester force-pushed the validation-errs-enhancements branch from 1c8ecef to 8cb3adc Compare September 10, 2020 09:56

vdemeester force-pushed the validation-errs-enhancements branch from 8cb3adc to 39a21ec Compare September 10, 2020 10:48

vdemeester commented Sep 10, 2020

View reviewed changes

vdemeester force-pushed the validation-errs-enhancements branch from 39a21ec to 772f289 Compare September 10, 2020 11:07

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 10, 2020

vdemeester commented Sep 10, 2020

View reviewed changes

vdemeester force-pushed the validation-errs-enhancements branch from 772f289 to e75fa6b Compare September 10, 2020 11:24

vdemeester commented Sep 10, 2020

View reviewed changes

vdemeester force-pushed the validation-errs-enhancements branch from e75fa6b to 2fa0253 Compare September 10, 2020 11:29

vdemeester force-pushed the validation-errs-enhancements branch from 2fa0253 to cfec788 Compare September 10, 2020 11:39

vdemeester changed the title ~~Enhance v1beta1 validation code 🍸~~ Enhance v1beta1 validation code for task 🍸 Sep 10, 2020

vdemeester force-pushed the validation-errs-enhancements branch from cfec788 to 0ae1c0a Compare September 10, 2020 11:46

vdemeester force-pushed the validation-errs-enhancements branch from 0ae1c0a to 5764051 Compare September 10, 2020 12:43

vdemeester added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Sep 11, 2020

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2020

pritidesai reviewed Sep 15, 2020

View reviewed changes

pritidesai approved these changes Sep 18, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2020

pritidesai reviewed Sep 18, 2020

View reviewed changes

savitaashture reviewed Sep 21, 2020

View reviewed changes

tekton-robot assigned savitaashture Sep 21, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2020

vdemeester force-pushed the validation-errs-enhancements branch from 5764051 to 7e08dc0 Compare September 21, 2020 08:39

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2020

tekton-robot merged commit 7b2b258 into tektoncd:master Sep 21, 2020

vdemeester deleted the validation-errs-enhancements branch September 21, 2020 14:16

vdemeester mentioned this pull request Sep 21, 2020

Enhance v1beta1 validation code for clustertask 🍸 #3268

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance v1beta1 validation code for task 🍸 #3185

Enhance v1beta1 validation code for task 🍸 #3185

vdemeester commented Sep 9, 2020 •

edited

Loading

vdemeester commented Sep 9, 2020

vdemeester commented Sep 9, 2020

tekton-robot commented Sep 9, 2020

tekton-robot commented Sep 10, 2020

vdemeester Sep 10, 2020

tekton-robot commented Sep 10, 2020

vdemeester Sep 10, 2020

pritidesai Sep 18, 2020

tekton-robot commented Sep 10, 2020

vdemeester Sep 10, 2020

tekton-robot commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

vdemeester commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

vdemeester commented Sep 14, 2020

pritidesai Sep 15, 2020

vdemeester Sep 15, 2020

pritidesai left a comment

tekton-robot commented Sep 18, 2020

pritidesai Sep 18, 2020

savitaashture left a comment

tekton-robot commented Sep 21, 2020

savitaashture commented Sep 21, 2020

ghost commented Sep 21, 2020

savitaashture commented Sep 21, 2020


		func (tr TaskResult) Validate(_ context.Context) *apis.FieldError {

Enhance v1beta1 validation code for task 🍸 #3185

Enhance v1beta1 validation code for task 🍸 #3185

Conversation

vdemeester commented Sep 9, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

vdemeester commented Sep 9, 2020

vdemeester commented Sep 9, 2020

tekton-robot commented Sep 9, 2020

tekton-robot commented Sep 10, 2020

vdemeester Sep 10, 2020

Choose a reason for hiding this comment

tekton-robot commented Sep 10, 2020

vdemeester Sep 10, 2020

Choose a reason for hiding this comment

pritidesai Sep 18, 2020

Choose a reason for hiding this comment

tekton-robot commented Sep 10, 2020

vdemeester Sep 10, 2020

Choose a reason for hiding this comment

tekton-robot commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

vdemeester commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

tekton-robot commented Sep 10, 2020

vdemeester commented Sep 14, 2020

pritidesai Sep 15, 2020

Choose a reason for hiding this comment

vdemeester Sep 15, 2020

Choose a reason for hiding this comment

pritidesai left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 18, 2020

pritidesai Sep 18, 2020

Choose a reason for hiding this comment

savitaashture left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 21, 2020

savitaashture commented Sep 21, 2020

ghost commented Sep 21, 2020

savitaashture commented Sep 21, 2020

vdemeester commented Sep 9, 2020 •

edited

Loading