Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Correct missing field(s) error #2295

Merged
merged 5 commits into from
Mar 27, 2020
Merged

Correct missing field(s) error #2295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3061def
Correct missing field(s) error
GregDritschler Mar 26, 2020
9c8dc17
Merge branch 'master' into issue2235
GregDritschler Mar 26, 2020
a2b3f19
Correct missing field(s) error
GregDritschler Mar 26, 2020
431c0d5
Correct missing field(s) error
GregDritschler Mar 26, 2020
ca0d345
Correct missing field(s) error
GregDritschler Mar 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1alpha1/pipeline_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func validateParamResults(tasks []PipelineTask) error {
// of Tasks expressed in the Pipeline makes sense.
func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError {
if equality.Semantic.DeepEqual(ps, &PipelineSpec{}) {
return apis.ErrMissingField(apis.CurrentField)
return apis.ErrGeneric("expected at least one, got none", "spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces")
}

// Names cannot be duplicated
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ func TestPipeline_Validate(t *testing.T) {
tb.PipelineTask("foo", "foo-task"),
)),
failureExpected: true,
}, {
name: "pipeline spec missing",
p: tb.Pipeline("pipeline", "namespace"),
failureExpected: true,
}, {
name: "pipeline spec invalid (duplicate tasks)",
p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec(
Expand Down
4 changes: 0 additions & 4 deletions pkg/apis/pipeline/v1alpha1/task_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/validate"
"github.com/tektoncd/pipeline/pkg/substitution"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/util/validation"
"knative.dev/pkg/apis"
)
Expand All @@ -41,9 +40,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 {
return apis.ErrMissingField("steps")
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1alpha1/task_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,10 +365,10 @@ func TestTaskSpecValidateError(t *testing.T) {
fields fields
expectedError apis.FieldError
}{{
name: "nil",
name: "empty spec",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 🙏 thankyou!

expectedError: apis.FieldError{
Message: `missing field(s)`,
Paths: []string{""},
Paths: []string{"steps"},
},
}, {
name: "no build",
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/pipeline_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func validateGraph(tasks []PipelineTask) error {
// of Tasks expressed in the Pipeline makes sense.
func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError {
if equality.Semantic.DeepEqual(ps, &PipelineSpec{}) {
return apis.ErrMissingField(apis.CurrentField)
return apis.ErrGeneric("expected at least one, got none", "spec.description", "spec.params", "spec.resources", "spec.tasks", "spec.workspaces")
}

// Names cannot be duplicated
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ func TestPipeline_Validate(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: "asdf123456789012345678901234567890123456789012345678901234567890"},
},
failureExpected: true,
}, {
name: "pipeline spec missing",
p: &v1beta1.Pipeline{
ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
},
failureExpected: true,
}, {
name: "pipeline spec missing taskref and taskspec",
p: &v1beta1.Pipeline{
Expand Down
4 changes: 0 additions & 4 deletions pkg/apis/pipeline/v1beta1/task_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/validate"
"github.com/tektoncd/pipeline/pkg/substitution"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/util/validation"
"knative.dev/pkg/apis"
)
Expand All @@ -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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return apis.ErrMissingField("steps")
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1beta1/task_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,10 @@ func TestTaskSpecValidateError(t *testing.T) {
fields fields
expectedError apis.FieldError
}{{
name: "nil",
name: "empty spec",
expectedError: apis.FieldError{
Message: `missing field(s)`,
Paths: []string{""},
Paths: []string{"steps"},
},
}, {
name: "no step",
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/resource/v1alpha1/pipelineresource_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (r *PipelineResource) Validate(ctx context.Context) *apis.FieldError {

func (rs *PipelineResourceSpec) Validate(ctx context.Context) *apis.FieldError {
if equality.Semantic.DeepEqual(rs, &PipelineResourceSpec{}) {
return apis.ErrMissingField(apis.CurrentField)
return apis.ErrMissingField("spec.type")
}
if rs.Type == PipelineResourceTypeCluster {
var authFound, cadataFound, isInsecure bool
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/resource/v1alpha1/pipelineresource_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ func TestResourceValidation_Invalid(t *testing.T) {
},
},
want: apis.ErrInvalidValue("spec.type", "not-supported"),
}, {
name: "missing spec",
res: &v1alpha1.PipelineResource{},
want: apis.ErrMissingField("spec.type"),
},
}
for _, tt := range tests {
Expand Down