diff --git a/pkg/cmd/explain/explain.go b/pkg/cmd/explain/explain.go index 827d0b74..172a883b 100644 --- a/pkg/cmd/explain/explain.go +++ b/pkg/cmd/explain/explain.go @@ -68,10 +68,6 @@ type ExplainOptions struct { Mapper meta.RESTMapper Schema openapi.Resources - // Toggles whether the OpenAPI v3 template-based renderer should be used to show - // output. - EnableOpenAPIV3 bool - // Name of the template to use with the openapiv3 template renderer. If // `EnableOpenAPIV3` is disabled, this does nothing OutputFormat string @@ -82,10 +78,9 @@ type ExplainOptions struct { func NewExplainOptions(parent string, streams genericclioptions.IOStreams) *ExplainOptions { return &ExplainOptions{ - IOStreams: streams, - CmdParent: parent, - EnableOpenAPIV3: cmdutil.ExplainOpenapiV3.IsEnabled(), - OutputFormat: plaintextTemplateName, + IOStreams: streams, + CmdParent: parent, + OutputFormat: plaintextTemplateName, } } @@ -109,9 +104,7 @@ func NewCmdExplain(parent string, f cmdutil.Factory, streams genericclioptions.I cmd.Flags().StringVar(&o.APIVersion, "api-version", o.APIVersion, "Get different explanations for particular API version (API group/version)") // Only enable --output as a valid flag if the feature is enabled - if o.EnableOpenAPIV3 { - cmd.Flags().StringVar(&o.OutputFormat, "output", plaintextTemplateName, "Format in which to render the schema (plaintext, plaintext-openapiv2)") - } + cmd.Flags().StringVar(&o.OutputFormat, "output", plaintextTemplateName, "Format in which to render the schema (plaintext, plaintext-openapiv2)") return cmd } @@ -128,12 +121,10 @@ func (o *ExplainOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args [] return err } - // Only openapi v3 needs the openapiv3client - if o.EnableOpenAPIV3 { - o.OpenAPIV3Client, err = f.OpenAPIV3Client() - if err != nil { - return err - } + // Only openapi v3 needs the discovery client. + o.OpenAPIV3Client, err = f.OpenAPIV3Client() + if err != nil { + return err } o.args = args @@ -172,39 +163,36 @@ func (o *ExplainOptions) Run() error { } // Fallback to openapiv2 implementation using special template name - if o.EnableOpenAPIV3 { - switch o.OutputFormat { - case plaintextOpenAPIV2TemplateName: + switch o.OutputFormat { + case plaintextOpenAPIV2TemplateName: + return o.renderOpenAPIV2(fullySpecifiedGVR, fieldsPath) + case plaintextTemplateName: + // Check whether the server reponds to OpenAPIV3. + if _, err := o.OpenAPIV3Client.Paths(); err != nil { + // Use v2 renderer if server does not support v3 return o.renderOpenAPIV2(fullySpecifiedGVR, fieldsPath) - case plaintextTemplateName: - // Check whether the server reponds to OpenAPIV3. - if _, err := o.OpenAPIV3Client.Paths(); err != nil { - // Use v2 renderer if server does not support v3 - return o.renderOpenAPIV2(fullySpecifiedGVR, fieldsPath) - } - - fallthrough - default: - if len(o.APIVersion) > 0 { - apiVersion, err := schema.ParseGroupVersion(o.APIVersion) - if err != nil { - return err - } - fullySpecifiedGVR.Group = apiVersion.Group - fullySpecifiedGVR.Version = apiVersion.Version - } - - return openapiv3explain.PrintModelDescription( - fieldsPath, - o.Out, - o.OpenAPIV3Client, - fullySpecifiedGVR, - o.Recursive, - o.OutputFormat, - ) } + + fallthrough + default: + if len(o.APIVersion) > 0 { + apiVersion, err := schema.ParseGroupVersion(o.APIVersion) + if err != nil { + return err + } + fullySpecifiedGVR.Group = apiVersion.Group + fullySpecifiedGVR.Version = apiVersion.Version + } + + return openapiv3explain.PrintModelDescription( + fieldsPath, + o.Out, + o.OpenAPIV3Client, + fullySpecifiedGVR, + o.Recursive, + o.OutputFormat, + ) } - return o.renderOpenAPIV2(fullySpecifiedGVR, fieldsPath) } func (o *ExplainOptions) renderOpenAPIV2( diff --git a/pkg/cmd/explain/explain_test.go b/pkg/cmd/explain/explain_test.go index 045d4bac..d5347dde 100644 --- a/pkg/cmd/explain/explain_test.go +++ b/pkg/cmd/explain/explain_test.go @@ -22,7 +22,6 @@ import ( "regexp" "testing" - "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/meta" sptest "k8s.io/apimachinery/pkg/util/strategicpatch/testing" "k8s.io/cli-runtime/pkg/genericclioptions" @@ -278,21 +277,3 @@ func runExplainTestCases(t *testing.T, cases []explainTestCase) { buf.Reset() } } - -func TestAlphaEnablement(t *testing.T) { - alphas := map[cmdutil.FeatureGate]string{ - cmdutil.ExplainOpenapiV3: "output", - } - for feature, flag := range alphas { - f := cmdtesting.NewTestFactory() - defer f.Cleanup() - - cmd := explain.NewCmdExplain("kubectl", f, genericclioptions.NewTestIOStreamsDiscard()) - require.Nil(t, cmd.Flags().Lookup(flag), "flag %q should not be registered without the %q feature enabled", flag, feature) - - cmdtesting.WithAlphaEnvs([]cmdutil.FeatureGate{feature}, t, func(t *testing.T) { - cmd := explain.NewCmdExplain("kubectl", f, genericclioptions.NewTestIOStreamsDiscard()) - require.NotNil(t, cmd.Flags().Lookup(flag), "flag %q should be registered with the %q feature enabled", flag, feature) - }) - } -} diff --git a/pkg/explain/v2/explain.go b/pkg/explain/v2/explain.go index e88054b6..43db7337 100644 --- a/pkg/explain/v2/explain.go +++ b/pkg/explain/v2/explain.go @@ -18,6 +18,7 @@ package v2 import ( "encoding/json" + "errors" "fmt" "io" @@ -85,5 +86,12 @@ func printModelDescriptionWithGenerator( return fmt.Errorf("failed to parse openapi schema for %s: %w", resourcePath, err) } - return generator.Render(outputFormat, parsedV3Schema, gvr, fieldsPath, recursive, w) + err = generator.Render(outputFormat, parsedV3Schema, gvr, fieldsPath, recursive, w) + + explainErr := explainError("") + if errors.As(err, &explainErr) { + return explainErr + } + + return err } diff --git a/pkg/explain/v2/funcs.go b/pkg/explain/v2/funcs.go index 60740d8d..f67a9f4c 100644 --- a/pkg/explain/v2/funcs.go +++ b/pkg/explain/v2/funcs.go @@ -29,8 +29,18 @@ import ( "k8s.io/kubectl/pkg/util/term" ) +type explainError string + +func (e explainError) Error() string { + return string(e) +} + func WithBuiltinTemplateFuncs(tmpl *template.Template) *template.Template { return tmpl.Funcs(map[string]interface{}{ + "throw": func(e string, args ...any) (string, error) { + errString := fmt.Sprintf(e, args...) + return "", explainError(errString) + }, "toJson": func(obj any) (string, error) { res, err := json.Marshal(obj) return string(res), err diff --git a/pkg/explain/v2/templates/plaintext.tmpl b/pkg/explain/v2/templates/plaintext.tmpl index c0a5ffad..09fe4406 100644 --- a/pkg/explain/v2/templates/plaintext.tmpl +++ b/pkg/explain/v2/templates/plaintext.tmpl @@ -19,10 +19,10 @@ {{- with include "schema" (dict "gvk" $gvk "Document" $.Document "FieldPath" $.FieldPath "Recursive" $.Recursive) -}} {{- . -}} {{- else -}} - error: GVK {{$gvk}} not found in OpenAPI schema + {{- throw "error: GVK %v not found in OpenAPI schema" $gvk -}} {{- end -}} {{- else -}} - error: GVR ({{$.GVR.String}}) not found in OpenAPI schema + {{- throw "error: GVR (%v) not found in OpenAPI schema" $.GVR.String -}} {{- end -}} {{- "\n" -}} @@ -43,7 +43,8 @@ Takes dictionary as argument with keys: {{- with include "output" (set $ "schema" .) -}} {{- . -}} {{- else -}} - error: field "{{$.FieldPath}}" does not exist + {{- $fieldName := (index $.FieldPath (sub (len $.FieldPath) 1)) -}} + {{- throw "error: field \"%v\" does not exist" $fieldName}} {{- end -}} {{- break -}} {{- end -}} @@ -88,7 +89,7 @@ Takes dictionary as argument with keys: {{- $subschema := index $resolved.properties (first $.FieldPath) -}} {{- if eq 1 (len $.FieldPath) -}} {{- /* TODO: The original explain would say RESOURCE instead of FIELD here under some circumstances */ -}} - FIELD: {{first $.FieldPath}} <{{ template "typeGuess" (dict "schema" $subschema) }}>{{"\n"}} + FIELD: {{first $.FieldPath}} <{{ template "typeGuess" (dict "schema" $subschema "Document" $.Document) }}>{{"\n"}} {{- "\n" -}} {{- end -}} {{- template "output" (set $nextContext "history" (dict) "FieldPath" (slice $.FieldPath 1) "schema" $subschema ) -}} @@ -149,7 +150,7 @@ Takes dictionary as argument with keys: {{- $nextContext := set $ "history" (set $.history $refString 1) -}} {{- $resolved := or (resolveRef $refString $.Document) $.schema -}} {{- range $k, $v := $resolved.properties -}} - {{- template "fieldDetail" (dict "name" $k "schema" $resolved "short" $.Recursive "level" $.level) -}} + {{- template "fieldDetail" (dict "name" $k "schema" $resolved "short" $.Recursive "level" $.level "Document" $.Document) -}} {{- if $.Recursive -}} {{- /* Check if we already know about this element */}} {{- template "fieldList" (set $nextContext "schema" $v "level" (add $.level 1)) -}} @@ -174,12 +175,13 @@ Takes dictionary as argument with keys: name: name of field short: limit printing to a single-line summary level: indentation amount + Document: openapi document */ -}} {{- define "fieldDetail" -}} {{- $level := or $.level 0 -}} {{- $indentAmount := mul $level 2 -}} {{- $fieldSchema := index $.schema.properties $.name -}} - {{- $.name | indent $indentAmount -}}{{"\t"}}<{{ template "typeGuess" (dict "schema" $fieldSchema) }}> + {{- $.name | indent $indentAmount -}}{{"\t"}}<{{ template "typeGuess" (dict "schema" $fieldSchema "Document" $.Document) }}> {{- if contains $.schema.required $.name}} -required-{{- end -}} {{- "\n" -}} {{- if not $.short -}} @@ -225,24 +227,35 @@ Takes dictionary as argument with keys: Takes dictionary as argument with keys: schema: openapiv3 JSON schema + Document: openapi document */ -}} {{- define "typeGuess" -}} {{- with $.schema -}} {{- if .items -}} - []{{template "typeGuess" (dict "schema" .items)}} + []{{template "typeGuess" (set $ "schema" .items)}} {{- else if .additionalProperties -}} - map[string]{{template "typeGuess" (dict "schema" .additionalProperties)}} + map[string]{{template "typeGuess" (set $ "schema" .additionalProperties)}} {{- else if and .allOf (not .properties) (eq (len .allOf) 1) -}} {{- /* If allOf has a single element and there are no direct properties on the schema, defer to the allOf */ -}} - {{- template "typeGuess" (dict "schema" (first .allOf)) -}} + {{- template "typeGuess" (set $ "schema" (first .allOf)) -}} {{- else if index . "$ref"}} {{- /* Parse the #!/components/schemas/io.k8s.Kind string into just the Kind name */ -}} {{- $ref := index . "$ref" -}} - {{- $name := split $ref "/" | last -}} - {{- or (split $name "." | last) "Object" -}} + {{- /* Look up ref schema to see primitive type. Only put the ref type name if it is an object. */ -}} + {{- $refSchema := resolveRef $ref $.Document -}} + {{- if (or (not $refSchema) (or (not $refSchema.type) (eq $refSchema.type "object"))) -}} + {{- $name := split $ref "/" | last -}} + {{- or (split $name "." | last) "Object" -}} + {{- else if $refSchema.type -}} + {{- or $refSchema.type "Object" -}} + {{- else -}} + {{- or .type "Object" -}} + {{- end -}} {{- else -}} - {{- or .type "Object" -}} + {{/* Old explain used capitalized "Object". Just follow suit */}} + {{- if eq .type "object" -}}Object + {{- else -}}{{- or .type "Object" -}}{{- end -}} {{- end -}} {{- else -}} {{- fail "expected schema argument to subtemplate 'typeguess'" -}} diff --git a/pkg/explain/v2/templates/plaintext_test.go b/pkg/explain/v2/templates/plaintext_test.go index a282f583..f7ab947b 100644 --- a/pkg/explain/v2/templates/plaintext_test.go +++ b/pkg/explain/v2/templates/plaintext_test.go @@ -66,21 +66,21 @@ type testCase struct { } type check interface { - doCheck(output string) error + doCheck(output string, err error) error } type checkError string -func (c checkError) doCheck(output string) error { - if !strings.Contains(output, "error: "+string(c)) { - return fmt.Errorf("expected error: '%v' in string:\n%v", string(c), output) +func (c checkError) doCheck(output string, err error) error { + if !strings.Contains(err.Error(), "error: "+string(c)) { + return fmt.Errorf("expected error: '%v' in string:\n%v", string(c), err) } return nil } type checkContains string -func (c checkContains) doCheck(output string) error { +func (c checkContains) doCheck(output string, err error) error { if !strings.Contains(output, string(c)) { return fmt.Errorf("expected substring: '%v' in string:\n%v", string(c), output) } @@ -89,7 +89,7 @@ func (c checkContains) doCheck(output string) error { type checkEquals string -func (c checkEquals) doCheck(output string) error { +func (c checkEquals) doCheck(output string, err error) error { if output != string(c) { return fmt.Errorf("output is not equal to expectation:\n%v", cmp.Diff(string(c), output)) } @@ -123,7 +123,7 @@ func TestPlaintext(t *testing.T) { Recursive: false, }, Checks: []check{ - checkError("GVR (/, Resource=) not found in OpenAPI schema\n"), + checkError("GVR (/, Resource=) not found in OpenAPI schema"), }, }, { @@ -158,7 +158,7 @@ func TestPlaintext(t *testing.T) { Recursive: false, }, Checks: []check{ - checkError(`field "[does not exist]" does not exist`), + checkError(`field "exist" does not exist`), }, }, { @@ -295,6 +295,30 @@ func TestPlaintext(t *testing.T) { checkEquals("string"), }, }, + { + // Show that a ref to a primitive type uses the referred type's type + Name: "PrimitiveRef", + Subtemplate: "typeGuess", + Context: map[string]any{ + "schema": map[string]any{ + "description": "a cool field", + "$ref": "#/components/schemas/v1.Time", + }, + "Document": map[string]any{ + "components": map[string]any{ + "schemas": map[string]any{ + "v1.Time": map[string]any{ + "type": "string", + "format": "date-time", + }, + }, + }, + }, + }, + Checks: []check{ + checkEquals("string"), + }, + }, { // Shows that the typeguess template behaves correctly given an // array with unknown items @@ -310,6 +334,20 @@ func TestPlaintext(t *testing.T) { checkEquals("array"), }, }, + { + // Shows that the typeguess puts Object tpye in title case + Name: "ObjectTitle", + Subtemplate: "typeGuess", + Context: map[string]any{ + "schema": map[string]any{ + "description": "a cool field", + "type": "object", + }, + }, + Checks: []check{ + checkEquals("Object"), + }, + }, { // Shows that the typeguess template works with scalars Name: "ArrayOfScalar", @@ -526,16 +564,17 @@ func TestPlaintext(t *testing.T) { t.Run(testName, func(t *testing.T) { buf := bytes.NewBuffer(nil) + + var outputErr error if len(tcase.Subtemplate) == 0 { - tmpl.Execute(buf, tcase.Context) + outputErr = tmpl.Execute(buf, tcase.Context) } else { - tmpl.ExecuteTemplate(buf, tcase.Subtemplate, tcase.Context) + outputErr = tmpl.ExecuteTemplate(buf, tcase.Subtemplate, tcase.Context) } - require.NoError(t, err) output := buf.String() for _, check := range tcase.Checks { - err = check.doCheck(output) + err = check.doCheck(output, outputErr) if err != nil { t.Log("test failed on output:\n" + output)