Correct missing field(s) error #2295

GregDritschler · 2020-03-26T13:59:28Z

Changes

As I mentioned in the issue, it is debatable whether a completely empty pipeline spec is an error since all fields are optional. On the other hand there is value in trying to catch mistakes. It's unlikely anyone wants to define a pipeline with nothing in it. So I changed the code to return the following error.

	if equality.Semantic.DeepEqual(ps, &PipelineSpec{}) {
		return apis.ErrGeneric("expected at least one, got none", "spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces")
	}

field_error.go doesn't have any methods with canned text for this situation. I found uses of ErrGeneric with the text "expected at least one, got none" in some places under vendor/knative.dev and decided that was good text for this case. It produces the following error:

Error from server (BadRequest): error when creating "pipelinemissingspec1.yaml": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: expected at least one, got none: spec.description, spec.params, spec.resources, spec.tasks, spec.workspaces

I looked for other resource validation methods that use apis.ErrMissingField(apis.CurrentField).

task_validation.go:
Immediately after checking for an empty spec, the validation method checks if any steps are defined. Therefore the empty spec check is superfluous and I removed it.
pipelineresource_validation.go:
I changed this one to specify the path that's required ("spec.type").
condition_validation.go:
I could not reproduce the ErrMissingField error with condition. I left the code alone to be safe.
workspace_validation.go:
I could not reproduce the ErrMissingField error with workspace. I left the code alone to be safe.

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

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.

Fixes #2235 Fixes #1844

tekton-robot · 2020-03-26T13:59:43Z

Hi @GregDritschler. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester · 2020-03-26T14:19:26Z

/ok-to-test

tekton-robot · 2020-03-26T14:22:05Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	96.3%	97.1%	0.7
pkg/apis/pipeline/v1alpha1/task_validation.go	83.9%	83.7%	-0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	93.8%	94.5%	0.8
pkg/apis/pipeline/v1beta1/task_validation.go	83.6%	83.3%	-0.2
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	86.8%	88.7%	1.9

GregDritschler · 2020-03-26T17:58:50Z

/retest

tekton-robot · 2020-03-26T18:00:32Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	96.3%	97.1%	0.7
pkg/apis/pipeline/v1alpha1/task_validation.go	83.9%	83.7%	-0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	93.8%	94.5%	0.8
pkg/apis/pipeline/v1beta1/task_validation.go	83.6%	83.3%	-0.2
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	86.8%	88.7%	1.9

GregDritschler · 2020-03-26T19:19:09Z

/retest

tekton-robot · 2020-03-26T19:20:34Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	96.3%	97.1%	0.7
pkg/apis/pipeline/v1alpha1/task_validation.go	83.9%	83.7%	-0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	93.8%	94.5%	0.8
pkg/apis/pipeline/v1beta1/task_validation.go	83.6%	83.3%	-0.2
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	86.8%	88.7%	1.9

GregDritschler · 2020-03-26T19:39:23Z

/retest

tekton-robot · 2020-03-26T19:41:35Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	96.3%	97.1%	0.7
pkg/apis/pipeline/v1alpha1/task_validation.go	83.9%	83.7%	-0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	93.8%	94.5%	0.8
pkg/apis/pipeline/v1beta1/task_validation.go	83.6%	83.3%	-0.2
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	86.8%	88.7%	1.9

vdemeester

/meow

tekton-robot · 2020-03-27T07:54:31Z

@vdemeester:

In response to this:

/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2020-03-27T07:54:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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~~ [vdemeester]

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

vdemeester · 2020-03-27T07:55:23Z

/cc @sbwsg
Do we want this in 0.11 ? 👼

ghost · 2020-03-27T14:19:19Z

sure thing! added cherry pick label.

ghost · 2020-03-27T14:23:40Z

pkg/apis/pipeline/v1beta1/task_validation_test.go

@@ -285,7 +285,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 		name: "nil",


i know this isn't part of the diff but it's incorrectly named - the Task Spec does not end up being nil in this test. It ends up being a valid pointer to an empty TaskSpec object.

It doesn't look like we exercise the nil case.

I changed the name.

ghost · 2020-03-27T14:27:25Z

pkg/apis/pipeline/v1beta1/task_validation.go

@@ -40,9 +39,6 @@ func (t *Task) Validate(ctx context.Context) *apis.FieldError {
 }

 func (ts *TaskSpec) Validate(ctx context.Context) *apis.FieldError {
-	if equality.Semantic.DeepEqual(ts, &TaskSpec{}) {
-		return apis.ErrMissingField(apis.CurrentField)
-	}

 	if len(ts.Steps) == 0 {


What will happen if ts is a nil pointer? Is there protection against that elsewhere in the codebase that will prevent it from happening?

The way the code is now, the callers of the validate methods appear to be responsible for checking for nil. For example in taskrun_validation:

// Validate TaskSpec if it's present if ts.TaskSpec != nil { if err := ts.TaskSpec.Validate(ctx); err != nil { return err } }

I haven't seen a nil check inside any of the validation methods I've looked at.

I'm not sure this is always the case though - see Task.Validate a few lines above this one:

func (t *Task) Validate(ctx context.Context) *apis.FieldError { if err := validate.ObjectMetadata(t.GetObjectMeta()); err != nil { return err.ViaField("metadata") } return t.Spec.Validate(ctx) }

I'm fine to remove the check in TaskSpec.Validate if we're absolutely sure that there won't be usages. But if we're cherry-picking this into the 0.11 branch just before we cut the beta and we're not 100% sure then I'd prefer we postpone this change to another PR. WDYT?

(Also, I believe - though I could be completely wrong! - that equality.Semantic.DeepEqual will actually consider a nil pointer and &TaskSpec{} as semantically equal, so this would return an error if there was a nil pointer. We should have unit testing around this.)

No, the first thing I did was to check if the DeepEqual check was catching a nil. I put the check back and forced ts to be nil. It fell through the DeepEqual check and panicked on the ts.Steps reference. So I don't believe I'm introducing an exposure. It was there before.

(fwiw I don't think this is fixing an urgent problem so it's fine to postpone merging it to post 0.11)

👍 understood, thanks for testing the nil case and correcting my assumption here.

GregDritschler · 2020-03-27T15:39:04Z

/retest

tekton-robot · 2020-03-27T15:41:01Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	96.3%	97.1%	0.7
pkg/apis/pipeline/v1alpha1/task_validation.go	83.9%	83.7%	-0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	93.8%	94.5%	0.8
pkg/apis/pipeline/v1beta1/task_validation.go	83.6%	83.3%	-0.2
pkg/apis/resource/v1alpha1/pipelineresource_validation.go	86.8%	88.7%	1.9

ghost · 2020-03-27T15:58:35Z

pkg/apis/pipeline/v1alpha1/task_validation_test.go

@@ -365,10 +365,10 @@ func TestTaskSpecValidateError(t *testing.T) {
 		fields        fields
 		expectedError apis.FieldError
 	}{{
-		name: "nil",
+		name: "empty spec",


nice 🙏 thankyou!

ghost · 2020-03-27T19:46:20Z

/lgtm

Correct missing field(s) error

3061def

Fixes #2235 Fixes #1844

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 26, 2020

tekton-robot requested review from bobcatfish and dlorenc March 26, 2020 13:59

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 26, 2020

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 26, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 26, 2020

Merge branch 'master' into issue2235

9c8dc17

Correct missing field(s) error

a2b3f19

Correct missing field(s) error

431c0d5

vdemeester approved these changes Mar 27, 2020

View reviewed changes

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

tekton-robot requested a review from a user March 27, 2020 07:55

ghost added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Mar 27, 2020

ghost reviewed Mar 27, 2020

View reviewed changes

Correct missing field(s) error

ca0d345

ghost reviewed Mar 27, 2020

View reviewed changes

tekton-robot assigned ghost Mar 27, 2020

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

tekton-robot merged commit 0b3a8b1 into tektoncd:master Mar 27, 2020

ghost removed the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Mar 30, 2020

afrittoli added the kind/bug Categorizes issue or PR as related to a bug. label Apr 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct missing field(s) error #2295

Correct missing field(s) error #2295

GregDritschler commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

vdemeester commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

GregDritschler commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

GregDritschler commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

GregDritschler commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

vdemeester left a comment

tekton-robot commented Mar 27, 2020

tekton-robot commented Mar 27, 2020

vdemeester commented Mar 27, 2020

ghost commented Mar 27, 2020

ghost Mar 27, 2020

GregDritschler Mar 27, 2020

ghost Mar 27, 2020

GregDritschler Mar 27, 2020

ghost Mar 27, 2020

ghost Mar 27, 2020

GregDritschler Mar 27, 2020

GregDritschler Mar 27, 2020

ghost Mar 27, 2020

GregDritschler commented Mar 27, 2020

tekton-robot commented Mar 27, 2020

ghost Mar 27, 2020

ghost commented Mar 27, 2020

		@@ -285,7 +285,7 @@ func TestTaskSpecValidateError(t *testing.T) {
		name: "nil",

Correct missing field(s) error #2295

Correct missing field(s) error #2295

Conversation

GregDritschler commented Mar 26, 2020

Changes

Submitter Checklist

Reviewer Notes

tekton-robot commented Mar 26, 2020

vdemeester commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

GregDritschler commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

GregDritschler commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

GregDritschler commented Mar 26, 2020

tekton-robot commented Mar 26, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 27, 2020

tekton-robot commented Mar 27, 2020

vdemeester commented Mar 27, 2020

ghost commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GregDritschler commented Mar 27, 2020

tekton-robot commented Mar 27, 2020

Choose a reason for hiding this comment

ghost commented Mar 27, 2020