From 124eb96e4574d632924e676d5bac244ea2aa658e Mon Sep 17 00:00:00 2001 From: Nikita Vaniasin Date: Wed, 29 Nov 2023 14:27:08 +0100 Subject: [PATCH 1/2] Fix doc and schema validation for shared objects - proper parsing for FileSet tokens - use full package path for field names - ignore zz_generated files during parsing --- docs/api/ArangoMLExtension.V1Alpha1.md | 6 +-- docs/api/ArangoMLStorage.V1Alpha1.md | 12 +++--- internal/cr_validation_test.go | 14 ++++++- internal/docs_parser_test.go | 38 +++++++++---------- internal/docs_test.go | 13 +++---- internal/schema_builder_test.go | 2 +- pkg/crd/crds/ml-storage.schema.generated.yaml | 6 +++ 7 files changed, 51 insertions(+), 40 deletions(-) diff --git a/docs/api/ArangoMLExtension.V1Alpha1.md b/docs/api/ArangoMLExtension.V1Alpha1.md index 682582131..3c08286f2 100644 --- a/docs/api/ArangoMLExtension.V1Alpha1.md +++ b/docs/api/ArangoMLExtension.V1Alpha1.md @@ -48,7 +48,7 @@ ArangoPipeDatabase define Database name to be used as MetadataService Backend ### .status.metadataService.secret.name -Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/ml/v1alpha1/batchjob_spec.go#L29) +Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/shared/v1/object.go#L32) Name of the object @@ -56,7 +56,7 @@ Name of the object ### .status.metadataService.secret.namespace -Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/ml/v1alpha1/batchjob_status.go#L5) +Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/shared/v1/object.go#L35) Namespace of the object. Should default to the namespace of the parent object @@ -64,7 +64,7 @@ Namespace of the object. Should default to the namespace of the parent object ### .status.metadataService.secret.uid -Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/ml/v1alpha1/batchjob_status.go#L7) +Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/shared/v1/object.go#L38) UID keeps the information about object UID diff --git a/docs/api/ArangoMLStorage.V1Alpha1.md b/docs/api/ArangoMLStorage.V1Alpha1.md index 13d22115c..dfb0ca7c3 100644 --- a/docs/api/ArangoMLStorage.V1Alpha1.md +++ b/docs/api/ArangoMLStorage.V1Alpha1.md @@ -23,7 +23,7 @@ Required ### .spec.backend.s3.caSecret.name -Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/ml/v1alpha1/batchjob_spec.go#L29) +Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/shared/v1/object.go#L32) Name of the object @@ -31,7 +31,7 @@ Name of the object ### .spec.backend.s3.caSecret.namespace -Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/ml/v1alpha1/batchjob_status.go#L5) +Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/shared/v1/object.go#L35) Namespace of the object. Should default to the namespace of the parent object @@ -39,7 +39,7 @@ Namespace of the object. Should default to the namespace of the parent object ### .spec.backend.s3.caSecret.uid -Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/ml/v1alpha1/batchjob_status.go#L7) +Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/shared/v1/object.go#L38) UID keeps the information about object UID @@ -47,7 +47,7 @@ UID keeps the information about object UID ### .spec.backend.s3.credentialsSecret.name -Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/ml/v1alpha1/batchjob_spec.go#L29) +Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/shared/v1/object.go#L32) Name of the object @@ -55,7 +55,7 @@ Name of the object ### .spec.backend.s3.credentialsSecret.namespace -Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/ml/v1alpha1/batchjob_status.go#L5) +Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/shared/v1/object.go#L35) Namespace of the object. Should default to the namespace of the parent object @@ -63,7 +63,7 @@ Namespace of the object. Should default to the namespace of the parent object ### .spec.backend.s3.credentialsSecret.uid -Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/ml/v1alpha1/batchjob_status.go#L7) +Type: `string` [\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.2.35/pkg/apis/shared/v1/object.go#L38) UID keeps the information about object UID diff --git a/internal/cr_validation_test.go b/internal/cr_validation_test.go index 382a7a4b8..262edcf9b 100644 --- a/internal/cr_validation_test.go +++ b/internal/cr_validation_test.go @@ -22,6 +22,7 @@ package internal import ( "fmt" + "go/token" "os" "path" "reflect" @@ -63,6 +64,10 @@ func Test_GenerateCRValidationSchemas(t *testing.T) { obj interface{} } + fset := token.NewFileSet() + + sharedFields := parseSourceFiles(t, root, fset, fmt.Sprintf("%s/pkg/apis/shared/v1", root)) + // CR file prefix -> packages to parse -> versions -> obj input := map[string]map[string]map[string]genSpec{ "apps-job": { @@ -207,10 +212,15 @@ func Test_GenerateCRValidationSchemas(t *testing.T) { for filePrefix, packagesToVersion := range input { validationPerVersion := make(map[string]apiextensions.CustomResourceValidation, len(packagesToVersion)) for apiDir, versionMap := range packagesToVersion { - fields, fileSets := parseSourceFiles(t, apiDir) + fields := parseSourceFiles(t, root, fset, apiDir) + + for n, f := range sharedFields { + require.NotContains(t, fields, n) + fields[n] = f + } for version, generationSpec := range versionMap { - sb := newSchemaBuilder(root, fields, fileSets) + sb := newSchemaBuilder(root, fields, fset) s := sb.TypeToSchema(t, reflect.TypeOf(generationSpec.obj), ".spec") require.NotNil(t, s, version) diff --git a/internal/docs_parser_test.go b/internal/docs_parser_test.go index 6a7e3df49..f3538b85f 100644 --- a/internal/docs_parser_test.go +++ b/internal/docs_parser_test.go @@ -29,7 +29,6 @@ import ( "path/filepath" "reflect" "strings" - "sync" "testing" "github.com/stretchr/testify/require" @@ -38,6 +37,10 @@ import ( "github.com/arangodb/kube-arangodb/pkg/util" ) +const ( + rootPackageName = "github.com/arangodb/kube-arangodb" +) + func parseDocDefinition(t *testing.T, root, path, typ string, field *ast.Field, fs *token.FileSet) DocDefinition { def := DocDefinition{ Path: path, @@ -163,7 +166,7 @@ func iterateOverObjectDirect(t *testing.T, fields map[string]*ast.Field, name st continue } - fullFieldName := fmt.Sprintf("%s.%s", object.String(), f.Name) + fullFieldName := fmt.Sprintf("%s.%s.%s", object.PkgPath(), object.Name(), f.Name) doc, ok := fields[fullFieldName] if !ok && !f.Anonymous { @@ -272,19 +275,9 @@ type parsedSource struct { fs *token.FileSet } -var parsedSourcesCache = make(map[string]parsedSource) -var parsedSourcesCacheLock sync.Mutex - // parseSourceFiles returns map of -> AST for structure Field and the token inspector for all files in package -func parseSourceFiles(t *testing.T, path string) (map[string]*ast.Field, *token.FileSet) { - parsedSourcesCacheLock.Lock() - defer parsedSourcesCacheLock.Unlock() - - if ret, ok := parsedSourcesCache[path]; ok { - return ret.fields, ret.fs - } - - d, fs := parseMultipleDirs(t, parser.ParseComments, path) +func parseSourceFiles(t *testing.T, root string, fset *token.FileSet, path string) map[string]*ast.Field { + d := parseMultipleDirs(t, root, fset, parser.ParseComments, path) r := map[string]*ast.Field{} @@ -323,26 +316,29 @@ func parseSourceFiles(t *testing.T, path string) (map[string]*ast.Field, *token. }) } - parsedSourcesCache[path] = parsedSource{fields: r, fs: fs} - return r, fs + return r } -func parseMultipleDirs(t *testing.T, mode parser.Mode, dirs ...string) (map[string]*ast.Package, *token.FileSet) { - fset := token.NewFileSet() // positions are relative to fset +func parseMultipleDirs(t *testing.T, root string, fset *token.FileSet, mode parser.Mode, dirs ...string) map[string]*ast.Package { + // positions are relative to fset r := map[string]*ast.Package{} for _, dir := range dirs { d, err := parser.ParseDir(fset, dir, func(info fs.FileInfo) bool { - return !strings.HasSuffix(info.Name(), "_test.go") + return !strings.HasSuffix(info.Name(), "_test.go") && + info.Name() != "zz_generated.deepcopy.go" }, mode) require.NoError(t, err) - for k, v := range d { + require.Len(t, d, 1) + k := strings.ReplaceAll(dir, root, rootPackageName) + + for _, v := range d { require.NotContains(t, r, k) r[k] = v } } - return r, fset + return r } diff --git a/internal/docs_test.go b/internal/docs_test.go index 06579a642..79f438c81 100644 --- a/internal/docs_test.go +++ b/internal/docs_test.go @@ -128,7 +128,9 @@ func Test_GenerateAPIDocs(t *testing.T) { root := os.Getenv("ROOT") require.NotEmpty(t, root) - sharedFields, sharedFilesSet := parseSourceFiles(t, fmt.Sprintf("%s/pkg/apis/shared/v1", root)) + fset := token.NewFileSet() + + sharedFields := parseSourceFiles(t, root, fset, fmt.Sprintf("%s/pkg/apis/shared/v1", root)) // package path -> result doc file name -> name of the top-level field to be described -> field instance for reflection input := map[string]map[string]map[string]interface{}{ @@ -182,17 +184,14 @@ func Test_GenerateAPIDocs(t *testing.T) { resultPaths := make(map[string]string) for apiDir, docs := range input { - fields, fileSet := parseSourceFiles(t, apiDir) + fields := parseSourceFiles(t, root, fset, apiDir) for n, f := range sharedFields { + require.NotContains(t, fields, n) fields[n] = f } - sharedFilesSet.Iterate(func(file *token.File) bool { - fileSet.AddFile(file.Name(), fileSet.Base()+file.Base(), file.Size()) - return true - }) - util.CopyMap(resultPaths, generateDocs(t, docs, fields, fileSet)) + util.CopyMap(resultPaths, generateDocs(t, docs, fields, fset)) } generateIndex(t, resultPaths) } diff --git a/internal/schema_builder_test.go b/internal/schema_builder_test.go index eeb7932cc..1b35540fc 100644 --- a/internal/schema_builder_test.go +++ b/internal/schema_builder_test.go @@ -191,7 +191,7 @@ func (b *schemaBuilder) StructToSchema(t *testing.T, structObj reflect.Type, pat s := b.TypeToSchema(t, f.Type, p) require.NotNil(t, s, p) - fullFieldName := fmt.Sprintf("%s.%s", structObj.String(), f.Name) + fullFieldName := fmt.Sprintf("%s.%s.%s", structObj.PkgPath(), structObj.Name(), f.Name) def := b.lookupDefinition(t, fullFieldName, p) if def != nil { def.ApplyToSchema(s) diff --git a/pkg/crd/crds/ml-storage.schema.generated.yaml b/pkg/crd/crds/ml-storage.schema.generated.yaml index 5f6c536cb..d92a025df 100644 --- a/pkg/crd/crds/ml-storage.schema.generated.yaml +++ b/pkg/crd/crds/ml-storage.schema.generated.yaml @@ -25,10 +25,13 @@ v1alpha1: - `ca.key` PEM encoded private key of the CA certificate properties: name: + description: Name of the object type: string namespace: + description: Namespace of the object. Should default to the namespace of the parent object type: string uid: + description: UID keeps the information about object UID type: string type: object credentialsSecret: @@ -37,10 +40,13 @@ v1alpha1: Required properties: name: + description: Name of the object type: string namespace: + description: Namespace of the object. Should default to the namespace of the parent object type: string uid: + description: UID keeps the information about object UID type: string type: object endpoint: From 284473a473f5af385d2cdf4b88b6bc7d7528c95b Mon Sep 17 00:00:00 2001 From: Nikita Vaniasin Date: Wed, 29 Nov 2023 14:39:31 +0100 Subject: [PATCH 2/2] Cleanup --- internal/docs_parser_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/docs_parser_test.go b/internal/docs_parser_test.go index f3538b85f..d1a0d6e46 100644 --- a/internal/docs_parser_test.go +++ b/internal/docs_parser_test.go @@ -270,11 +270,6 @@ func extractTag(tag string) (string, bool) { return parts[0], false } -type parsedSource struct { - fields map[string]*ast.Field - fs *token.FileSet -} - // parseSourceFiles returns map of -> AST for structure Field and the token inspector for all files in package func parseSourceFiles(t *testing.T, root string, fset *token.FileSet, path string) map[string]*ast.Field { d := parseMultipleDirs(t, root, fset, parser.ParseComments, path)