From 8027191b25da748d5babad1fceb689aecc2967ac Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Fri, 30 Nov 2018 16:42:06 -0800 Subject: [PATCH 1/5] Simplify results config by removing `log` key Since currently the only kind of result is `log`, we can remove that key. We change change this if we decide to have other kinds of results later. --- examples/run/output-pipeline-run.yaml | 7 +++-- examples/run/pipeline-run.yaml | 7 +++-- examples/run/task-run.yaml | 4 +++ .../pipeline/v1alpha1/pipelinerun_types.go | 10 ++----- .../v1alpha1/pipelinerun_validation.go | 4 +-- .../v1alpha1/pipelinerun_validation_test.go | 21 +++++---------- .../pipeline/v1alpha1/taskrun_validation.go | 14 +++------- .../v1alpha1/taskrun_validation_test.go | 26 ++++++------------- .../v1alpha1/zz_generated.deepcopy.go | 17 ------------ 9 files changed, 33 insertions(+), 77 deletions(-) diff --git a/examples/run/output-pipeline-run.yaml b/examples/run/output-pipeline-run.yaml index fcfd6008643..d0b8a30c148 100644 --- a/examples/run/output-pipeline-run.yaml +++ b/examples/run/output-pipeline-run.yaml @@ -10,10 +10,9 @@ spec: type: manual serviceAccount: 'default' results: - logs: - name: 'logBucket' - type: 'gcs' - url: 'gcs://somebucket/results/logs' + name: 'logBucket' + type: 'gcs' + url: 'gcs://somebucket/results/logs' resources: - name: first-create-file inputs: diff --git a/examples/run/pipeline-run.yaml b/examples/run/pipeline-run.yaml index e410e4fcb03..50afab27e77 100644 --- a/examples/run/pipeline-run.yaml +++ b/examples/run/pipeline-run.yaml @@ -10,10 +10,9 @@ spec: type: manual serviceAccount: 'default' results: - logs: - name: 'logBucket' - type: 'gcs' - url: 'gcs://somebucket/results/logs' + name: 'logBucket' + type: 'gcs' + url: 'gcs://somebucket/results/logs' resources: - name: build-skaffold-web inputs: diff --git a/examples/run/task-run.yaml b/examples/run/task-run.yaml index 280a49bcca3..fbf9639fd4c 100644 --- a/examples/run/task-run.yaml +++ b/examples/run/task-run.yaml @@ -8,6 +8,10 @@ spec: trigger: triggerRef: type: manual + results: + name: 'logBucket' + type: 'gcs' + url: 'gcs://somebucket/results/logs' inputs: resources: - name: workspace diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go index 3ed4727bf6e..474f6ae1a47 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go @@ -104,15 +104,9 @@ const ( ResultTargetTypeGCS = "gcs" ) -// Results tells a pipeline where to persist the results of runnign the pipeline. -type Results struct { - // Logs will store all logs output from running a task. - Logs ResultTarget `json:"logs"` -} - -// ResultTarget is used to identify an endpoint where results can be uploaded. The +// Results is used to identify an endpoint where results can be uploaded. The // serviceaccount used for the pipeline must have access to this endpoint. -type ResultTarget struct { +type Results struct { Name string `json:"name"` Type ResultTargetType `json:"type"` URL string `json:"url"` diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go index 842659fbc4b..3f54167f6b2 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go @@ -46,10 +46,10 @@ func (ps *PipelineRunSpec) Validate() *apis.FieldError { if ps.Results != nil { // Results.Logs should have a valid URL and ResultTargetType - if err := validateURL(ps.Results.Logs.URL, "pipelinerun.spec.Results.Logs.URL"); err != nil { + if err := validateURL(ps.Results.URL, "pipelinerun.spec.Results.URL"); err != nil { return err } - if err := validateResultTargetType(ps.Results.Logs.Type, "pipelinerun.spec.Results.Logs.Type"); err != nil { + if err := validateResultTargetType(ps.Results.Type, "pipelinerun.spec.Results.Type"); err != nil { return err } } diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go index 8da5e396dec..c561d048de5 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go @@ -25,13 +25,6 @@ import ( var validURL = "http://www.google.com" -func validResultTarget(name string) ResultTarget { - return ResultTarget{ - URL: validURL, - Name: name, - Type: "gcs", - } -} func TestPipelineRun_Invalidate(t *testing.T) { tests := []struct { name string @@ -101,15 +94,13 @@ func TestPipelineRun_Invalidate(t *testing.T) { Type: PipelineTriggerTypeManual, }, Results: &Results{ - Logs: ResultTarget{ - Name: "runs", - URL: "badurl", - Type: "gcs", - }, + Name: "runs", + URL: "badurl", + Type: "gcs", }, }, }, - want: apis.ErrInvalidValue("badurl", "pipelinerun.spec.Results.Logs.URL"), + want: apis.ErrInvalidValue("badurl", "pipelinerun.spec.Results.URL"), }, } @@ -136,7 +127,9 @@ func TestPipelineRun_Validate(t *testing.T) { Type: "manual", }, Results: &Results{ - Logs: validResultTarget("logs"), + URL: validURL, + Name: "logs", + Type: "gcs", }, }, } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go index af9a245ee94..30d236dfb2b 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go @@ -67,13 +67,6 @@ func (ts *TaskRunSpec) Validate() *apis.FieldError { return ts.Results.Validate("spec.results") } -func (r *Results) Validate(path string) *apis.FieldError { - if err := r.Logs.Validate(fmt.Sprintf("%s.logs", path)); err != nil { - return err - } - return nil -} - func (i TaskRunInputs) Validate(path string) *apis.FieldError { if err := checkForPipelineResourceDuplicates(i.Resources, fmt.Sprintf("%s.Resources.Name", path)); err != nil { return err @@ -85,9 +78,10 @@ func (o TaskRunOutputs) Validate(path string) *apis.FieldError { return checkForPipelineResourceDuplicates(o.Resources, fmt.Sprintf("%s.Resources.Name", path)) } -func (r ResultTarget) Validate(path string) *apis.FieldError { - // if result target is not set then do not error - var emptyTarget = ResultTarget{} +// Validate will validate the result configuration, if it is present. +func (r Results) Validate(path string) *apis.FieldError { + // if result is not set then do not error + var emptyTarget = Results{} if r == emptyTarget { return nil } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go index 4719fc7b389..5189b81dfec 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go @@ -310,43 +310,35 @@ func TestResult_Invalidate(t *testing.T) { { name: "invalid task type result", result: &Results{ - Logs: ResultTarget{ - Name: "resultlogs", - Type: "wrongtype", - }, + Name: "resultlogs", + Type: "wrongtype", }, - wantErr: apis.ErrInvalidValue("wrongtype", "spec.results.logs.Type"), + wantErr: apis.ErrInvalidValue("wrongtype", "spec.results.Type"), }, { name: "invalid task type result name", result: &Results{ - Logs: ResultTarget{ - Name: "", - Type: ResultTargetTypeGCS, - }, + Name: "", + Type: ResultTargetTypeGCS, }, - wantErr: apis.ErrMissingField("spec.results.logs.name"), + wantErr: apis.ErrMissingField("spec.results.name"), }, { name: "invalid task type result url", result: &Results{ - Logs: ResultTarget{ Name: "resultrunname", Type: ResultTargetTypeGCS, URL: "", - }, }, - wantErr: apis.ErrMissingField("spec.results.logs.URL"), + wantErr: apis.ErrMissingField("spec.results.URL"), }, { name: "invalid task type result url", result: &Results{ - Logs: ResultTarget{ Name: "resultrunname", Type: "badtype", - }, }, - wantErr: apis.ErrInvalidValue("badtype", "spec.results.logs.Type"), + wantErr: apis.ErrInvalidValue("badtype", "spec.results.Type"), }, } @@ -362,11 +354,9 @@ func TestResult_Invalidate(t *testing.T) { func TestResultTarget_Validate(t *testing.T) { rs := &Results{ - Logs: ResultTarget{ Name: "testname", Type: ResultTargetTypeGCS, URL: "http://github.com", - }, } if err := rs.Validate("spec.results"); err != nil { t.Errorf("ResultTarget.Validate() error = %v", err) diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go index d45e506b380..eb2fa586ccd 100644 --- a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go @@ -639,26 +639,9 @@ func (in *ResourceDependency) DeepCopy() *ResourceDependency { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ResultTarget) DeepCopyInto(out *ResultTarget) { - *out = *in - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResultTarget. -func (in *ResultTarget) DeepCopy() *ResultTarget { - if in == nil { - return nil - } - out := new(ResultTarget) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Results) DeepCopyInto(out *Results) { *out = *in - out.Logs = in.Logs return } From 9953d225f6cfdda14abd38606ea9b7681c401238 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Fri, 30 Nov 2018 17:34:54 -0800 Subject: [PATCH 2/5] Consolidate logic for handling results config for PipelineRun + TaskRun It turned out TaskRun and PipelienRun were using slightly different logic for validating the results config, even tho it should be the same for both. Now they share the logic and in both cases the config is optional. --- .../pipeline/v1alpha1/pipelinerun_types.go | 22 +---- .../v1alpha1/pipelinerun_validation.go | 30 +----- .../v1alpha1/pipelinerun_validation_test.go | 25 +---- pkg/apis/pipeline/v1alpha1/result_types.go | 91 +++++++++++++++++ .../pipeline/v1alpha1/result_types_test.go | 99 +++++++++++++++++++ pkg/apis/pipeline/v1alpha1/taskrun_types.go | 3 +- .../pipeline/v1alpha1/taskrun_validation.go | 30 ++---- .../v1alpha1/taskrun_validation_test.go | 66 +------------ 8 files changed, 206 insertions(+), 160 deletions(-) create mode 100644 pkg/apis/pipeline/v1alpha1/result_types.go create mode 100644 pkg/apis/pipeline/v1alpha1/result_types_test.go diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go index 474f6ae1a47..228a5af6840 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go @@ -47,7 +47,7 @@ type PipelineRunSpec struct { // +optional ServiceAccount string `json:"serviceAccount"` // +optional - Results *Results `json:"results"` + Results *Results `json:"results,omitempty"` Generation int64 `json:"generation,omitempty"` } @@ -92,26 +92,6 @@ type PipelineRef struct { APIVersion string `json:"apiVersion,omitempty"` } -// AllResultTargetTypes is a list of all ResultTargetTypes, used for validation -var AllResultTargetTypes = []ResultTargetType{ResultTargetTypeGCS} - -// ResultTargetType represents the type of endpoint that this result target is, -// so that the controller will know how to write results to it. -type ResultTargetType string - -const ( - // ResultTargetTypeGCS indicates that the URL endpoint is a GCS bucket. - ResultTargetTypeGCS = "gcs" -) - -// Results is used to identify an endpoint where results can be uploaded. The -// serviceaccount used for the pipeline must have access to this endpoint. -type Results struct { - Name string `json:"name"` - Type ResultTargetType `json:"type"` - URL string `json:"url"` -} - // PipelineTriggerType indicates the mechanism by which this PipelineRun was created. type PipelineTriggerType string diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go index 3f54167f6b2..484b058ba59 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go @@ -17,8 +17,6 @@ limitations under the License. package v1alpha1 import ( - "net/url" - "github.com/knative/pkg/apis" "k8s.io/apimachinery/pkg/api/equality" ) @@ -43,36 +41,12 @@ func (ps *PipelineRunSpec) Validate() *apis.FieldError { if ps.PipelineTriggerRef.Type != PipelineTriggerTypeManual { return apis.ErrInvalidValue(string(ps.PipelineTriggerRef.Type), "pipelinerun.spec.triggerRef.type") } - + // check for results if ps.Results != nil { - // Results.Logs should have a valid URL and ResultTargetType - if err := validateURL(ps.Results.URL, "pipelinerun.spec.Results.URL"); err != nil { - return err - } - if err := validateResultTargetType(ps.Results.Type, "pipelinerun.spec.Results.Type"); err != nil { + if err := ps.Results.Validate("spec.results"); err != nil { return err } } return nil } - -func validateURL(u, path string) *apis.FieldError { - if u == "" { - return nil - } - _, err := url.ParseRequestURI(u) - if err != nil { - return apis.ErrInvalidValue(u, path) - } - return nil -} - -func validateResultTargetType(r ResultTargetType, path string) *apis.FieldError { - for _, a := range AllResultTargetTypes { - if a == r { - return nil - } - } - return apis.ErrInvalidValue(string(r), path) -} diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go index c561d048de5..7037e0fd595 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go @@ -23,8 +23,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -var validURL = "http://www.google.com" - func TestPipelineRun_Invalidate(t *testing.T) { tests := []struct { name string @@ -80,27 +78,6 @@ func TestPipelineRun_Invalidate(t *testing.T) { }, }, want: apis.ErrInvalidValue("badtype", "pipelinerun.spec.triggerRef.type"), - }, { - name: "invalid results", - pr: PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelineName", - }, - Spec: PipelineRunSpec{ - PipelineRef: PipelineRef{ - Name: "prname", - }, - PipelineTriggerRef: PipelineTriggerRef{ - Type: PipelineTriggerTypeManual, - }, - Results: &Results{ - Name: "runs", - URL: "badurl", - Type: "gcs", - }, - }, - }, - want: apis.ErrInvalidValue("badurl", "pipelinerun.spec.Results.URL"), }, } @@ -127,7 +104,7 @@ func TestPipelineRun_Validate(t *testing.T) { Type: "manual", }, Results: &Results{ - URL: validURL, + URL: "http://www.google.com", Name: "logs", Type: "gcs", }, diff --git a/pkg/apis/pipeline/v1alpha1/result_types.go b/pkg/apis/pipeline/v1alpha1/result_types.go new file mode 100644 index 00000000000..c0e361f05c2 --- /dev/null +++ b/pkg/apis/pipeline/v1alpha1/result_types.go @@ -0,0 +1,91 @@ +/* +Copyright 2018 The Knative Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + "fmt" + "net/url" + + "github.com/knative/pkg/apis" +) + +// AllResultTargetTypes is a list of all ResultTargetTypes, used for validation +var AllResultTargetTypes = []ResultTargetType{ResultTargetTypeGCS} + +// ResultTargetType represents the type of endpoint that this result target is, +// so that the controller will know how to write results to it. +type ResultTargetType string + +const ( + // ResultTargetTypeGCS indicates that the URL endpoint is a GCS bucket. + ResultTargetTypeGCS = "gcs" +) + +// Results is used to identify an endpoint where results can be uploaded. The +// serviceaccount used for the pipeline must have access to this endpoint. +type Results struct { + Name string `json:"name"` + Type ResultTargetType `json:"type"` + URL string `json:"url"` +} + +// Validate will validate the result configuration. The path is the path at which +// we found this instance of `Results` (since it is probably a member of another +// structure) and will be used to report any errors. +func (r *Results) Validate(path string) *apis.FieldError { + // If set then verify all variables pass the validation + if r.Name == "" { + return apis.ErrMissingField(fmt.Sprintf("%s.name", path)) + } + + if r.Type != ResultTargetTypeGCS { + return apis.ErrInvalidValue(string(r.Type), fmt.Sprintf("%s.Type", path)) + } + + if err := validateResultTargetType(r.Type, fmt.Sprintf("%s.Type", path)); err != nil { + return err + } + + if r.URL == "" { + return apis.ErrMissingField(fmt.Sprintf("%s.URL", path)) + } + + if err := validateURL(r.URL, fmt.Sprintf("%s.URL", path)); err != nil { + return err + } + return nil +} + +func validateURL(u, path string) *apis.FieldError { + if u == "" { + return nil + } + _, err := url.ParseRequestURI(u) + if err != nil { + return apis.ErrInvalidValue(u, path) + } + return nil +} + +func validateResultTargetType(r ResultTargetType, path string) *apis.FieldError { + for _, a := range AllResultTargetTypes { + if a == r { + return nil + } + } + return apis.ErrInvalidValue(string(r), path) +} diff --git a/pkg/apis/pipeline/v1alpha1/result_types_test.go b/pkg/apis/pipeline/v1alpha1/result_types_test.go new file mode 100644 index 00000000000..0b15758295f --- /dev/null +++ b/pkg/apis/pipeline/v1alpha1/result_types_test.go @@ -0,0 +1,99 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/knative/pkg/apis" +) + +func TestValidate(t *testing.T) { + results := Results{ + URL: "http://google.com", + Name: "logs", + Type: "gcs", + } + err := results.Validate("somepath") + if err != nil { + t.Fatalf("Did not expect error when validating valid Results but got %s", err) + } +} + +func TestValidate_Invalid(t *testing.T) { + tests := []struct { + name string + results *Results + want *apis.FieldError + }{ + { + name: "invalid task type result", + results: &Results{ + Name: "resultlogs", + URL: "http://www.google.com", + Type: "wrongtype", + }, + want: apis.ErrInvalidValue("wrongtype", "spec.results.Type"), + }, + { + name: "invalid task type results name", + results: &Results{ + Name: "", + URL: "http://www.google.com", + Type: ResultTargetTypeGCS, + }, + want: apis.ErrMissingField("spec.results.name"), + }, + { + name: "invalid task type results missing url", + results: &Results{ + Name: "resultsrunname", + Type: ResultTargetTypeGCS, + URL: "", + }, + want: apis.ErrMissingField("spec.results.URL"), + }, + { + name: "invalid task type results bad url", + results: &Results{ + Name: "resultsrunname", + Type: ResultTargetTypeGCS, + URL: "badurl", + }, + want: apis.ErrInvalidValue("badurl", "spec.results.URL"), + }, + { + name: "invalid task type results type", + results: &Results{ + Name: "resultsrunname", + Type: "badtype", + URL: "http://www.google.com", + }, + want: apis.ErrInvalidValue("badtype", "spec.results.Type"), + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + path := "spec.results" + err := tc.results.Validate(path) + if d := cmp.Diff(err.Error(), tc.want.Error()); d != "" { + t.Errorf("Results.Validate/%s (-want, +got) = %v", tc.name, d) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types.go b/pkg/apis/pipeline/v1alpha1/taskrun_types.go index 1fc504079a0..d08c9732744 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_types.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_types.go @@ -40,7 +40,8 @@ type TaskRunSpec struct { Inputs TaskRunInputs `json:"inputs,omitempty"` // +optional Outputs TaskRunOutputs `json:"outputs,omitempty"` - Results Results `json:"results"` + // +optional + Results *Results `json:"results,omitempty"` // +optional Generation int64 `json:"generation,omitempty"` // +optional diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go index 30d236dfb2b..18554d4b0d0 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go @@ -64,7 +64,12 @@ func (ts *TaskRunSpec) Validate() *apis.FieldError { } // check for results - return ts.Results.Validate("spec.results") + if ts.Results != nil { + if err := ts.Results.Validate("spec.results"); err != nil { + return err + } + } + return nil } func (i TaskRunInputs) Validate(path string) *apis.FieldError { @@ -78,29 +83,6 @@ func (o TaskRunOutputs) Validate(path string) *apis.FieldError { return checkForPipelineResourceDuplicates(o.Resources, fmt.Sprintf("%s.Resources.Name", path)) } -// Validate will validate the result configuration, if it is present. -func (r Results) Validate(path string) *apis.FieldError { - // if result is not set then do not error - var emptyTarget = Results{} - if r == emptyTarget { - return nil - } - - // If set then verify all variables pass the validation - if r.Name == "" { - return apis.ErrMissingField(fmt.Sprintf("%s.name", path)) - } - - if r.Type != ResultTargetTypeGCS { - return apis.ErrInvalidValue(string(r.Type), fmt.Sprintf("%s.Type", path)) - } - - if r.URL == "" { - return apis.ErrMissingField(fmt.Sprintf("%s.URL", path)) - } - return nil -} - func checkForPipelineResourceDuplicates(resources []TaskRunResource, path string) *apis.FieldError { encountered := map[string]struct{}{} for _, r := range resources { diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go index 5189b81dfec..669a643040e 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go @@ -186,6 +186,10 @@ func TestTaskRunSpec_Validate(t *testing.T) { Name: "testtrigger", }, }, + Results: &Results{ + URL: "http://www.google.com", + Type: "gcs", + }, }, }, { @@ -301,68 +305,6 @@ func TestInput_Invalidate(t *testing.T) { } } -func TestResult_Invalidate(t *testing.T) { - tests := []struct { - name string - result *Results - wantErr *apis.FieldError - }{ - { - name: "invalid task type result", - result: &Results{ - Name: "resultlogs", - Type: "wrongtype", - }, - wantErr: apis.ErrInvalidValue("wrongtype", "spec.results.Type"), - }, - { - name: "invalid task type result name", - result: &Results{ - Name: "", - Type: ResultTargetTypeGCS, - }, - wantErr: apis.ErrMissingField("spec.results.name"), - }, - { - name: "invalid task type result url", - result: &Results{ - Name: "resultrunname", - Type: ResultTargetTypeGCS, - URL: "", - }, - wantErr: apis.ErrMissingField("spec.results.URL"), - }, - { - name: "invalid task type result url", - result: &Results{ - Name: "resultrunname", - Type: "badtype", - }, - wantErr: apis.ErrInvalidValue("badtype", "spec.results.Type"), - }, - } - - for _, ts := range tests { - t.Run(ts.name, func(t *testing.T) { - err := ts.result.Validate("spec.results") - if d := cmp.Diff(err.Error(), ts.wantErr.Error()); d != "" { - t.Errorf("Validate/%s (-want, +got) = %v", ts.name, d) - } - }) - } -} - -func TestResultTarget_Validate(t *testing.T) { - rs := &Results{ - Name: "testname", - Type: ResultTargetTypeGCS, - URL: "http://github.com", - } - if err := rs.Validate("spec.results"); err != nil { - t.Errorf("ResultTarget.Validate() error = %v", err) - } -} - func TestOutput_Validate(t *testing.T) { i := TaskRunOutputs{ Resources: []TaskRunResource{{ From ae731703ed1935eae154ad1379916cd6ab1aa63c Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Sat, 1 Dec 2018 10:07:17 -0800 Subject: [PATCH 3/5] Remove `name` field from `Results` In the interests in keeping our spec as lean and simple as it can possibly be, at the moment there is no reason why our Results config needs to have a name, so let's remove it. This is the last unnecessary field we have in our yamls that I'm aware of, so this fixes #138 --- examples/run/output-pipeline-run.yaml | 1 - examples/run/pipeline-run.yaml | 1 - examples/run/task-run.yaml | 1 - .../v1alpha1/pipelinerun_validation_test.go | 1 - pkg/apis/pipeline/v1alpha1/result_types.go | 6 ------ pkg/apis/pipeline/v1alpha1/result_types_test.go | 14 -------------- .../pipeline/v1alpha1/zz_generated.deepcopy.go | 10 +++++++++- 7 files changed, 9 insertions(+), 25 deletions(-) diff --git a/examples/run/output-pipeline-run.yaml b/examples/run/output-pipeline-run.yaml index d0b8a30c148..c288ce3fe31 100644 --- a/examples/run/output-pipeline-run.yaml +++ b/examples/run/output-pipeline-run.yaml @@ -10,7 +10,6 @@ spec: type: manual serviceAccount: 'default' results: - name: 'logBucket' type: 'gcs' url: 'gcs://somebucket/results/logs' resources: diff --git a/examples/run/pipeline-run.yaml b/examples/run/pipeline-run.yaml index 50afab27e77..842508dcc41 100644 --- a/examples/run/pipeline-run.yaml +++ b/examples/run/pipeline-run.yaml @@ -10,7 +10,6 @@ spec: type: manual serviceAccount: 'default' results: - name: 'logBucket' type: 'gcs' url: 'gcs://somebucket/results/logs' resources: diff --git a/examples/run/task-run.yaml b/examples/run/task-run.yaml index fbf9639fd4c..51fdc42c17b 100644 --- a/examples/run/task-run.yaml +++ b/examples/run/task-run.yaml @@ -9,7 +9,6 @@ spec: triggerRef: type: manual results: - name: 'logBucket' type: 'gcs' url: 'gcs://somebucket/results/logs' inputs: diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go index 7037e0fd595..9a51277453a 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go @@ -105,7 +105,6 @@ func TestPipelineRun_Validate(t *testing.T) { }, Results: &Results{ URL: "http://www.google.com", - Name: "logs", Type: "gcs", }, }, diff --git a/pkg/apis/pipeline/v1alpha1/result_types.go b/pkg/apis/pipeline/v1alpha1/result_types.go index c0e361f05c2..88dced1d8b9 100644 --- a/pkg/apis/pipeline/v1alpha1/result_types.go +++ b/pkg/apis/pipeline/v1alpha1/result_types.go @@ -38,7 +38,6 @@ const ( // Results is used to identify an endpoint where results can be uploaded. The // serviceaccount used for the pipeline must have access to this endpoint. type Results struct { - Name string `json:"name"` Type ResultTargetType `json:"type"` URL string `json:"url"` } @@ -47,11 +46,6 @@ type Results struct { // we found this instance of `Results` (since it is probably a member of another // structure) and will be used to report any errors. func (r *Results) Validate(path string) *apis.FieldError { - // If set then verify all variables pass the validation - if r.Name == "" { - return apis.ErrMissingField(fmt.Sprintf("%s.name", path)) - } - if r.Type != ResultTargetTypeGCS { return apis.ErrInvalidValue(string(r.Type), fmt.Sprintf("%s.Type", path)) } diff --git a/pkg/apis/pipeline/v1alpha1/result_types_test.go b/pkg/apis/pipeline/v1alpha1/result_types_test.go index 0b15758295f..65f1ce47d25 100644 --- a/pkg/apis/pipeline/v1alpha1/result_types_test.go +++ b/pkg/apis/pipeline/v1alpha1/result_types_test.go @@ -26,7 +26,6 @@ import ( func TestValidate(t *testing.T) { results := Results{ URL: "http://google.com", - Name: "logs", Type: "gcs", } err := results.Validate("somepath") @@ -44,25 +43,14 @@ func TestValidate_Invalid(t *testing.T) { { name: "invalid task type result", results: &Results{ - Name: "resultlogs", URL: "http://www.google.com", Type: "wrongtype", }, want: apis.ErrInvalidValue("wrongtype", "spec.results.Type"), }, - { - name: "invalid task type results name", - results: &Results{ - Name: "", - URL: "http://www.google.com", - Type: ResultTargetTypeGCS, - }, - want: apis.ErrMissingField("spec.results.name"), - }, { name: "invalid task type results missing url", results: &Results{ - Name: "resultsrunname", Type: ResultTargetTypeGCS, URL: "", }, @@ -71,7 +59,6 @@ func TestValidate_Invalid(t *testing.T) { { name: "invalid task type results bad url", results: &Results{ - Name: "resultsrunname", Type: ResultTargetTypeGCS, URL: "badurl", }, @@ -80,7 +67,6 @@ func TestValidate_Invalid(t *testing.T) { { name: "invalid task type results type", results: &Results{ - Name: "resultsrunname", Type: "badtype", URL: "http://www.google.com", }, diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go index eb2fa586ccd..ee2061dd558 100644 --- a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go @@ -959,7 +959,15 @@ func (in *TaskRunSpec) DeepCopyInto(out *TaskRunSpec) { out.Trigger = in.Trigger in.Inputs.DeepCopyInto(&out.Inputs) in.Outputs.DeepCopyInto(&out.Outputs) - out.Results = in.Results + if in.Results != nil { + in, out := &in.Results, &out.Results + if *in == nil { + *out = nil + } else { + *out = new(Results) + **out = **in + } + } if in.TaskRef != nil { in, out := &in.TaskRef, &out.TaskRef if *in == nil { From a7b0e3a5e28b47742de82251baa502b71bf1c13a Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Sat, 1 Dec 2018 10:10:54 -0800 Subject: [PATCH 4/5] Add `Results` fields to `TaskRun` and `PipelineRun` status When a user kicks off a run, they will provide an endpoint to upload logs to (initial implementation will be in #107). The corresponding fields in `status` will indicate where the logs actually got uplaoded to. Once we actually get to #107, and especially once we start supporting endpoints other than GCS, we may find this isn't useful and remove it. Fixes #146 --- pkg/apis/pipeline/v1alpha1/pipelinerun_types.go | 2 ++ pkg/apis/pipeline/v1alpha1/taskrun_types.go | 3 +++ pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go | 2 ++ 3 files changed, 7 insertions(+) diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go index 228a5af6840..43f3e582fc1 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go @@ -111,6 +111,8 @@ type PipelineTriggerRef struct { // PipelineRunStatus defines the observed state of PipelineRun type PipelineRunStatus struct { Conditions duckv1alpha1.Conditions `json:"conditions"` + // In #107 should be updated to hold the location logs have been uploaded to + Results Results `json:"results"` // map of TaskRun Status with the taskRun name as the key //+optional TaskRuns map[string]TaskRunStatus `json:"taskRuns,omitempty"` diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types.go b/pkg/apis/pipeline/v1alpha1/taskrun_types.go index d08c9732744..658c2172245 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_types.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_types.go @@ -104,6 +104,9 @@ type TaskRunStatus struct { // +optional Conditions duckv1alpha1.Conditions `json:"conditions,omitempty"` + // In #107 should be updated to hold the location logs have been uploaded to + Results Results `json:"results"` + // PodName is the name of the pod responsible for executing this task's steps. PodName string `json:"podName"` diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go index ee2061dd558..8c60598a6da 100644 --- a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go @@ -454,6 +454,7 @@ func (in *PipelineRunStatus) DeepCopyInto(out *PipelineRunStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + out.Results = in.Results if in.TaskRuns != nil { in, out := &in.TaskRuns, &out.TaskRuns *out = make(map[string]TaskRunStatus, len(*in)) @@ -1009,6 +1010,7 @@ func (in *TaskRunStatus) DeepCopyInto(out *TaskRunStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + out.Results = in.Results if in.StartTime != nil { in, out := &in.StartTime, &out.StartTime if *in == nil { From 421dd8d6df7271763b59572020cffe60380d549d Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Mon, 3 Dec 2018 14:55:08 -0800 Subject: [PATCH 5/5] Make results in Run `status` optional If it's optional in the spec, it should probably be optional in the Status, since the behaviour would probably be not to upload the logs anywhere if no endpoint is provided. Possibly in the future these could become mandator, depending how we design the results store interface. --- .../pipeline/v1alpha1/pipelinerun_types.go | 3 ++- pkg/apis/pipeline/v1alpha1/taskrun_types.go | 3 ++- .../v1alpha1/zz_generated.deepcopy.go | 20 +++++++++++++++++-- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go index 43f3e582fc1..eb6a76dcb88 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go @@ -112,7 +112,8 @@ type PipelineTriggerRef struct { type PipelineRunStatus struct { Conditions duckv1alpha1.Conditions `json:"conditions"` // In #107 should be updated to hold the location logs have been uploaded to - Results Results `json:"results"` + // +optional + Results *Results `json:"results,omitempty"` // map of TaskRun Status with the taskRun name as the key //+optional TaskRuns map[string]TaskRunStatus `json:"taskRuns,omitempty"` diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types.go b/pkg/apis/pipeline/v1alpha1/taskrun_types.go index 658c2172245..25b84925cd6 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_types.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_types.go @@ -105,7 +105,8 @@ type TaskRunStatus struct { Conditions duckv1alpha1.Conditions `json:"conditions,omitempty"` // In #107 should be updated to hold the location logs have been uploaded to - Results Results `json:"results"` + // +optional + Results *Results `json:"results,omitempty"` // PodName is the name of the pod responsible for executing this task's steps. PodName string `json:"podName"` diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go index 8c60598a6da..7c33d5a90c6 100644 --- a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go @@ -454,7 +454,15 @@ func (in *PipelineRunStatus) DeepCopyInto(out *PipelineRunStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - out.Results = in.Results + if in.Results != nil { + in, out := &in.Results, &out.Results + if *in == nil { + *out = nil + } else { + *out = new(Results) + **out = **in + } + } if in.TaskRuns != nil { in, out := &in.TaskRuns, &out.TaskRuns *out = make(map[string]TaskRunStatus, len(*in)) @@ -1010,7 +1018,15 @@ func (in *TaskRunStatus) DeepCopyInto(out *TaskRunStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - out.Results = in.Results + if in.Results != nil { + in, out := &in.Results, &out.Results + if *in == nil { + *out = nil + } else { + *out = new(Results) + **out = **in + } + } if in.StartTime != nil { in, out := &in.StartTime, &out.StartTime if *in == nil {