From 45d180338f545bfb58f7cb9afb1a45ab681ff6e7 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Wed, 12 May 2021 13:42:32 -0700 Subject: [PATCH] improve kpt fn render error messages (#1955) --- .../missing-fn-image/.expected/config.yaml | 2 +- .../.expected/config.yaml | 5 +-- .../missing-fn-image/.expected/config.yaml | 2 +- .../.expected/diff.patch | 35 +++++++++++++++++++ .../no-pipeline-in-subpackage/.krmignore | 1 + .../no-pipeline-in-subpackage/Kptfile | 12 +++++++ .../no-pipeline-in-subpackage/db/Kptfile | 4 +++ .../db/resources.yaml} | 14 +++++++- internal/fnruntime/container.go | 4 +-- pkg/api/kptfile/v1alpha2/validation.go | 11 +++--- 10 files changed, 76 insertions(+), 14 deletions(-) create mode 100644 e2e/testdata/fn-render/no-pipeline-in-subpackage/.expected/diff.patch create mode 100644 e2e/testdata/fn-render/no-pipeline-in-subpackage/.krmignore create mode 100644 e2e/testdata/fn-render/no-pipeline-in-subpackage/Kptfile create mode 100644 e2e/testdata/fn-render/no-pipeline-in-subpackage/db/Kptfile rename e2e/testdata/fn-render/{resource-has-pkgname-prefix/.expected/config.yaml => no-pipeline-in-subpackage/db/resources.yaml} (75%) diff --git a/e2e/testdata/fn-eval/missing-fn-image/.expected/config.yaml b/e2e/testdata/fn-eval/missing-fn-image/.expected/config.yaml index c22d0b65f9..bde0350440 100644 --- a/e2e/testdata/fn-eval/missing-fn-image/.expected/config.yaml +++ b/e2e/testdata/fn-eval/missing-fn-image/.expected/config.yaml @@ -17,7 +17,7 @@ exitCode: 1 image: gcr.io/kpt-fn/dne # non-existing image args: namespace: staging -stdErr: 'failed to check function image existence: function image "gcr.io/kpt-fn/dne" doesn''t exist' +stdErr: "error: function image \"gcr.io/kpt-fn/dne\" doesn't exist" stdOut: | [RUNNING] "gcr.io/kpt-fn/dne" [FAIL] "gcr.io/kpt-fn/dne" diff --git a/e2e/testdata/fn-render/invalid-inline-fnconfig/.expected/config.yaml b/e2e/testdata/fn-render/invalid-inline-fnconfig/.expected/config.yaml index 778f285ba8..de59673705 100644 --- a/e2e/testdata/fn-render/invalid-inline-fnconfig/.expected/config.yaml +++ b/e2e/testdata/fn-render/invalid-inline-fnconfig/.expected/config.yaml @@ -13,7 +13,4 @@ # limitations under the License. exitCode: 1 -stdErr: "missing Resource metadata" -stdOut: | - [RUNNING] "gcr.io/kpt-fn/set-namespace:unstable" - [PASS] "gcr.io/kpt-fn/set-namespace:unstable" +stdErr: 'Error: invalid pipeline: function "gcr.io/kpt-fn/set-label:unstable": functionConfig must be a valid KRM resource with `apiVersion` and `kind` fields' diff --git a/e2e/testdata/fn-render/missing-fn-image/.expected/config.yaml b/e2e/testdata/fn-render/missing-fn-image/.expected/config.yaml index ae4f983db3..d6c6df1fa0 100644 --- a/e2e/testdata/fn-render/missing-fn-image/.expected/config.yaml +++ b/e2e/testdata/fn-render/missing-fn-image/.expected/config.yaml @@ -13,7 +13,7 @@ # limitations under the License. exitCode: 1 -stdErr: 'failed to check function image existence: function image "gcr.io/kpt-fn/set-label1" doesn''t exist:' +stdErr: "Error: function image \"gcr.io/kpt-fn/set-label1\" doesn't exist" stdOut: | [RUNNING] "gcr.io/kpt-fn/set-namespace:unstable" [PASS] "gcr.io/kpt-fn/set-namespace:unstable" diff --git a/e2e/testdata/fn-render/no-pipeline-in-subpackage/.expected/diff.patch b/e2e/testdata/fn-render/no-pipeline-in-subpackage/.expected/diff.patch new file mode 100644 index 0000000000..a3daac9216 --- /dev/null +++ b/e2e/testdata/fn-render/no-pipeline-in-subpackage/.expected/diff.patch @@ -0,0 +1,35 @@ +diff --git a/db/resources.yaml b/db/resources.yaml +index 10cd18b..a9dd224 100644 +--- a/db/resources.yaml ++++ b/db/resources.yaml +@@ -11,17 +11,29 @@ + # 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. +- + apiVersion: apps/v1 + kind: Deployment + metadata: + name: nginx-deployment ++ namespace: staging ++ labels: ++ tier: backend + spec: + replicas: 3 ++ selector: ++ matchLabels: ++ tier: backend ++ template: ++ metadata: ++ labels: ++ tier: backend + --- + apiVersion: custom.io/v1 + kind: Custom + metadata: + name: custom ++ namespace: staging ++ labels: ++ tier: backend + spec: + image: nginx:1.2.3 \ No newline at end of file diff --git a/e2e/testdata/fn-render/no-pipeline-in-subpackage/.krmignore b/e2e/testdata/fn-render/no-pipeline-in-subpackage/.krmignore new file mode 100644 index 0000000000..9d7a4007d6 --- /dev/null +++ b/e2e/testdata/fn-render/no-pipeline-in-subpackage/.krmignore @@ -0,0 +1 @@ +.expected diff --git a/e2e/testdata/fn-render/no-pipeline-in-subpackage/Kptfile b/e2e/testdata/fn-render/no-pipeline-in-subpackage/Kptfile new file mode 100644 index 0000000000..bd4605990b --- /dev/null +++ b/e2e/testdata/fn-render/no-pipeline-in-subpackage/Kptfile @@ -0,0 +1,12 @@ +apiVersion: kpt.dev/v1alpha2 +kind: Kptfile +metadata: + name: app +pipeline: + mutators: + - image: gcr.io/kpt-fn/set-namespace:unstable + configMap: + namespace: staging + - image: gcr.io/kpt-fn/set-label:unstable + configMap: + tier: backend \ No newline at end of file diff --git a/e2e/testdata/fn-render/no-pipeline-in-subpackage/db/Kptfile b/e2e/testdata/fn-render/no-pipeline-in-subpackage/db/Kptfile new file mode 100644 index 0000000000..e30e1348f1 --- /dev/null +++ b/e2e/testdata/fn-render/no-pipeline-in-subpackage/db/Kptfile @@ -0,0 +1,4 @@ +apiVersion: kpt.dev/v1alpha2 +kind: Kptfile +metadata: + name: app \ No newline at end of file diff --git a/e2e/testdata/fn-render/resource-has-pkgname-prefix/.expected/config.yaml b/e2e/testdata/fn-render/no-pipeline-in-subpackage/db/resources.yaml similarity index 75% rename from e2e/testdata/fn-render/resource-has-pkgname-prefix/.expected/config.yaml rename to e2e/testdata/fn-render/no-pipeline-in-subpackage/db/resources.yaml index 6c321de691..10cd18bf5c 100644 --- a/e2e/testdata/fn-render/resource-has-pkgname-prefix/.expected/config.yaml +++ b/e2e/testdata/fn-render/no-pipeline-in-subpackage/db/resources.yaml @@ -12,4 +12,16 @@ # See the License for the specific language governing permissions and # limitations under the License. -exitCode: 0 +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 3 +--- +apiVersion: custom.io/v1 +kind: Custom +metadata: + name: custom +spec: + image: nginx:1.2.3 diff --git a/internal/fnruntime/container.go b/internal/fnruntime/container.go index df6dff35e9..a488e5de72 100644 --- a/internal/fnruntime/container.go +++ b/internal/fnruntime/container.go @@ -77,7 +77,7 @@ func (f *ContainerFn) Run(reader io.Reader, writer io.Writer) error { // output err := f.prepareImage() if err != nil { - return fmt.Errorf("failed to check function image existence: %w", err) + return err } errSink := bytes.Buffer{} @@ -164,7 +164,7 @@ func (f *ContainerFn) prepareImage() error { defer cancel() cmd = exec.CommandContext(ctx, dockerBin, args...) if _, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("function image %q doesn't exist: %w", f.Image, err) + return fmt.Errorf("function image %q doesn't exist", f.Image) } return nil } diff --git a/pkg/api/kptfile/v1alpha2/validation.go b/pkg/api/kptfile/v1alpha2/validation.go index 514c065a6e..579bc995da 100644 --- a/pkg/api/kptfile/v1alpha2/validation.go +++ b/pkg/api/kptfile/v1alpha2/validation.go @@ -19,7 +19,6 @@ import ( "regexp" "strings" - "github.com/GoogleContainerTools/kpt/internal/errors" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -28,9 +27,8 @@ const ( ) func (kf *KptFile) Validate() error { - const op errors.Op = "v1alpha2.kptfile.validate" if err := kf.Pipeline.validate(); err != nil { - return errors.E(op, fmt.Errorf("pipeline is not valid: %w", err)) + return fmt.Errorf("invalid pipeline: %w", err) } // TODO: validate other fields return nil @@ -40,7 +38,6 @@ func (kf *KptFile) Validate() error { // 'mutators' and 'validators' share same schema and // they are valid if all functions in them are ALL valid. func (p *Pipeline) validate() error { - const op errors.Op = "v1alpha2.pipeline.validate" if p == nil { return nil } @@ -51,7 +48,7 @@ func (p *Pipeline) validate() error { f := fns[i] err := f.validate() if err != nil { - return errors.E(op, fmt.Errorf("function %q is invalid: %w", f.Image, err)) + return fmt.Errorf("function %q: %w", f.Image, err) } } return nil @@ -74,6 +71,10 @@ func (f *Function) validate() error { configFields = append(configFields, "configMap") } if !IsNodeZero(&f.Config) { + config := yaml.NewRNode(&f.Config) + if _, err := config.GetMeta(); err != nil { + return fmt.Errorf("functionConfig must be a valid KRM resource with `apiVersion` and `kind` fields") + } configFields = append(configFields, "config") } if len(configFields) > 1 {