Addressing second round of comments

Signed-off-by: David Elie-Dit-Cosaque <[email protected]>

Author: edcdavid

internal/patches.go

@@ -16,15 +16,10 @@ import (
 	"open-cluster-management.io/policy-generator-plugin/internal/types"
 )
 
-const (
-	localSchemaFileName = "schema.json"
-	kustomizeDir        = "kustomize"
-)
-
-type KustomizeJSON struct {
-	types.Filepath `json:"openapi,omitempty" yaml:"openapi,omitempty"`
-	Patches        []Patch  `json:"patches" yaml:"patches"`
-	Resources      []string `json:"resources" yaml:"resources"`
+type kustomizeFile struct {
+	OpenAPI   types.Filepath `json:"openapi,omitempty" yaml:"openapi,omitempty"`
+	Patches   []Patch        `json:"patches" yaml:"patches"`
+	Resources []string       `json:"resources" yaml:"resources"`
 }
 
 type Patch struct {
@@ -183,19 +178,22 @@ func setPatchDefaults(
 // the patched manifests. An error is returned if the patches can't be applied. This should be
 // run after the Validate method.
 func (m *manifestPatcher) ApplyPatches() ([]map[string]interface{}, error) {
-	kustomizeDir := "kustomize"
+	const (
+		localSchemaFileName = "schema.json"
+		kustomizeDir        = "kustomize"
+	)
 
 	// Create the file system in memory with the Kustomize YAML files
 	fSys := filesys.MakeFsInMemory()
 
-	err := InitializeInMemoryKustomizeDir(fSys, m.openAPI.Path)
+	err := initializeInMemoryKustomizeDir(fSys, m.openAPI.Path, kustomizeDir, localSchemaFileName)
 	if err != nil {
 		return nil, fmt.Errorf("failed to initialize Kustomize dir: %w", err)
 	}
 
-	kustomizationYAMLFile := KustomizeJSON{}
+	kustomizationYAMLFile := kustomizeFile{}
 	if m.openAPI.Path != "" {
-		kustomizationYAMLFile.Filepath.Path = localSchemaFileName
+		kustomizationYAMLFile.OpenAPI.Path = localSchemaFileName
 	}
 
 	options := []struct {
@@ -271,7 +269,9 @@ func (m *manifestPatcher) ApplyPatches() ([]map[string]interface{}, error) {
 }
 
 // Initializes the in-memory file system with base directory and open API schema
-func InitializeInMemoryKustomizeDir(fSys filesys.FileSystem, schema string) (err error) {
+func initializeInMemoryKustomizeDir(fSys filesys.FileSystem, schema,
+	kustomizeDir, localSchemaFileName string,
+) (err error) {
 	err = fSys.Mkdir(kustomizeDir)
 	if err != nil {
 		return fmt.Errorf("an unexpected error occurred when configuring Kustomize: %w", err)
@@ -282,12 +282,12 @@ func InitializeInMemoryKustomizeDir(fSys filesys.FileSystem, schema string) (err
 
 		schemaJSON, err := os.ReadFile(schema)
 		if err != nil {
-			return fmt.Errorf("unable to open file: %s, err: %w ", schema, err)
+			return fmt.Errorf("error reading file %s: %w ", schema, err)
 		}
 
 		err = fSys.WriteFile(path.Join(kustomizeDir, localSchemaFileName), schemaJSON)
 		if err != nil {
-			return fmt.Errorf("error writing schema, err: %w", err)
+			return fmt.Errorf("error writing schema: %w", err)
 		}
 	}
 

internal/patches_test.go

@@ -303,6 +303,11 @@ func TestApplyPatchesInvalidPatch(t *testing.T) {
 }
 
 func TestInitializeInMemoryKustomizeDir(t *testing.T) {
+	const (
+		localSchemaFileName = "schema.json"
+		kustomizeDir        = "kustomize"
+	)
+
 	tmpDir := t.TempDir()
 	testSchemaPath := filepath.Join(tmpDir, "myschema.json")
 
@@ -337,7 +342,8 @@ func TestInitializeInMemoryKustomizeDir(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if err := InitializeInMemoryKustomizeDir(tt.args.fSys, tt.args.schema); (err != nil) != tt.wantErr {
+			if err := initializeInMemoryKustomizeDir(tt.args.fSys, tt.args.schema,
+				kustomizeDir, localSchemaFileName); (err != nil) != tt.wantErr {
 				t.Errorf("InitializeInMemoryKustomizeDir() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})

internal/plugin.go

@@ -1030,11 +1030,18 @@ func (p *Plugin) assertValidConfig() error {
 				)
 			}
 
-			err = verifyManifestPath(p.baseDirectory, manifest.Path)
+			err = verifyFilePath(p.baseDirectory, manifest.Path, "manifest")
 			if err != nil {
 				return err
 			}
 
+			if manifest.OpenAPI.Path != "" {
+				err = verifyFilePath(p.baseDirectory, manifest.OpenAPI.Path, "openapi")
+				if err != nil {
+					return err
+				}
+			}
+
 			evalInterval := manifest.EvaluationInterval
 
 			// Verify that consolidated manifests fields match that of the policy configuration.

internal/plugin_test.go

@@ -4445,7 +4445,7 @@ func TestOpenAPIListPatch(t *testing.T) {
 
 	// Set CR file path and OpenAPI path with temporary directory
 	p.Policies[0].Manifests[0].Path = crRelativePath
-	p.Policies[0].Manifests[0].Filepath.Path = openAPIRelativePath
+	p.Policies[0].Manifests[0].OpenAPI.Path = openAPIRelativePath
 
 	// Check configuration
 	if err := p.assertValidConfig(); err != nil {
@@ -4544,7 +4544,7 @@ func TestOpenAPIListUnkownFieldsPatch(t *testing.T) {
 
 	// Set CR file path and OpenAPI path with temporary directory
 	p.Policies[0].Manifests[0].Path = crRelativePath
-	p.Policies[0].Manifests[0].Filepath.Path = openAPIRelativePath
+	p.Policies[0].Manifests[0].OpenAPI.Path = openAPIRelativePath
 
 	// Check configuration
 	if err := p.assertValidConfig(); err != nil {

internal/testdata/OpenAPI/cr-files/cr2.yaml

@@ -15,4 +15,3 @@ spec:
           LocalMaxHoldoverOffSet: 1500
           LocalHoldoverTimeout: 14400
           MaxInSpecOffset: 100
- 

internal/testdata/OpenAPI/openapi-schema.json

@@ -57,4 +57,4 @@
       ]
     }
   }
-}
+}

internal/testdata/OpenAPI/policy-generator2.yaml

@@ -43,5 +43,3 @@ policies:
                             pins: $e810_pins
                             settings:
                                 LocalHoldoverTimeout: 14401
-          openapi:
-            path: /home/deliedit/dev/cnf-features-deploy/ztp/tools/pgt2acm/test/openapi-schema.json

internal/types/types.go

@@ -53,21 +53,19 @@ type Manifest struct {
 	Path                       string                   `json:"path,omitempty" yaml:"path,omitempty"`
 	ExtraDependencies          []PolicyDependency       `json:"extraDependencies,omitempty" yaml:"extraDependencies,omitempty"`
 	IgnorePending              bool                     `json:"ignorePending,omitempty" yaml:"ignorePending,omitempty"`
-	Filepath                   `json:"openapi,omitempty" yaml:"openapi,omitempty"`
+	OpenAPI                    Filepath                 `json:"openapi,omitempty" yaml:"openapi,omitempty"`
 }
 
 type Filepath struct {
-	Path string `json:"path" yaml:"path"`
+	Path string `json:"path,omitempty" yaml:"path,omitempty"`
 }
 
-type (
-	NamespaceSelector struct {
-		Exclude          []string                           `json:"exclude,omitempty" yaml:"exclude,omitempty"`
-		Include          []string                           `json:"include,omitempty" yaml:"include,omitempty"`
-		MatchLabels      *map[string]string                 `json:"matchLabels,omitempty" yaml:"matchLabels,omitempty"`
-		MatchExpressions *[]metav1.LabelSelectorRequirement `json:"matchExpressions,omitempty" yaml:"matchExpressions,omitempty"`
-	}
-)
+type NamespaceSelector struct {
+	Exclude          []string                           `json:"exclude,omitempty" yaml:"exclude,omitempty"`
+	Include          []string                           `json:"include,omitempty" yaml:"include,omitempty"`
+	MatchLabels      *map[string]string                 `json:"matchLabels,omitempty" yaml:"matchLabels,omitempty"`
+	MatchExpressions *[]metav1.LabelSelectorRequirement `json:"matchExpressions,omitempty" yaml:"matchExpressions,omitempty"`
+}
 
 // Define String() so that the LabelSelector is dereferenced in the logs
 func (t NamespaceSelector) String() string {

internal/utils.go

@@ -123,7 +123,7 @@ func getManifests(policyConf *types.PolicyConfig) ([][]map[string]interface{}, e
 		}
 
 		if len(manifest.Patches) > 0 {
-			patcher := manifestPatcher{manifests: manifestFiles, patches: manifest.Patches, openAPI: manifest.Filepath}
+			patcher := manifestPatcher{manifests: manifestFiles, patches: manifest.Patches, openAPI: manifest.OpenAPI}
 			const errTemplate = `failed to process the manifest at "%s": %w`
 
 			err = patcher.Validate()
@@ -574,39 +574,39 @@ func unmarshalManifestBytes(manifestBytes []byte) ([]map[string]interface{}, err
 	return yamlDocs, nil
 }
 
-// verifyManifestPath verifies that the manifest path is in the directory tree under baseDirectory.
+// verifyFilePath verifies that the file path is in the directory tree under baseDirectory.
 // An error is returned if it is not or the paths couldn't be properly resolved.
-func verifyManifestPath(baseDirectory string, manifestPath string) error {
-	absPath, err := filepath.Abs(manifestPath)
+func verifyFilePath(baseDirectory string, filePath, fileType string) error {
+	absPath, err := filepath.Abs(filePath)
 	if err != nil {
-		return fmt.Errorf("could not resolve the manifest path %s to an absolute path", manifestPath)
+		return fmt.Errorf("could not resolve the %s path %s to an absolute path", fileType, filePath)
 	}
 
 	absPath, err = filepath.EvalSymlinks(absPath)
 	if err != nil {
-		return fmt.Errorf("could not resolve symlinks to the manifest path %s", manifestPath)
+		return fmt.Errorf("could not resolve symlinks to the %s path %s", fileType, filePath)
 	}
 
 	relPath, err := filepath.Rel(baseDirectory, absPath)
 	if err != nil {
 		return fmt.Errorf(
-			"could not resolve the manifest path %s to a relative path from the kustomization.yaml file",
-			manifestPath,
+			"could not resolve the %s path %s to a relative path from the kustomization.yaml file",
+			fileType, filePath,
 		)
 	}
 
 	if relPath == "." {
 		return fmt.Errorf(
-			"the manifest path %s may not refer to the same directory as the kustomization.yaml file",
-			manifestPath,
+			"the %s path %s may not refer to the same directory as the kustomization.yaml file",
+			fileType, filePath,
 		)
 	}
 
 	parDir := ".." + string(filepath.Separator)
 	if strings.HasPrefix(relPath, parDir) || relPath == ".." {
 		return fmt.Errorf(
-			"the manifest path %s is not in the same directory tree as the kustomization.yaml file",
-			manifestPath,
+			"the %s path %s is not in the same directory tree as the kustomization.yaml file",
+			fileType, filePath,
 		)
 	}
 

internal/utils_test.go

@@ -1604,7 +1604,7 @@ func TestVerifyManifestPath(t *testing.T) {
 		t.Run(
 			"manifestPath="+test.ManifestPath,
 			func(t *testing.T) {
-				err := verifyManifestPath(workingDir, test.ManifestPath)
+				err := verifyFilePath(workingDir, test.ManifestPath, "manifest")
 				if err == nil {
 					assertEqual(t, "", test.ExpectedErrMsg)
 				} else {