Merge pull request #3166 from phanimarupaka/DeferOpenAPICleanup

Defer openAPI cleanup

cmd/config/runner/runner.go

@@ -60,37 +60,34 @@ func (e ExecuteCmdOnPkgs) Execute() error {
 	}
 
 	for i := range pkgsPaths {
-		pkgPath := pkgsPaths[i]
-		// Add schema present in openAPI file for current package
-		if e.NeedOpenAPI {
-			if err := openapi.AddSchemaFromFile(filepath.Join(pkgPath, ext.KRMFileName())); err != nil {
-				return err
-			}
-		}
-
-		if !e.SkipPkgPathPrint {
-			fmt.Fprintf(e.Writer, "%s/\n", pkgPath)
-		}
-
-		err := e.CmdRunner.ExecuteCmd(e.Writer, pkgPath)
+		err := e.processPkg(pkgsPaths[i])
 		if err != nil {
 			return err
 		}
-
 		if i != len(pkgsPaths)-1 {
 			fmt.Fprint(e.Writer, "\n")
 		}
-
-		// Delete schema present in openAPI file for current package
-		if e.NeedOpenAPI {
-			if err := openapi.DeleteSchemaInFile(filepath.Join(pkgPath, ext.KRMFileName())); err != nil {
-				return err
-			}
-		}
 	}
 	return nil
 }
 
+func (e ExecuteCmdOnPkgs) processPkg(pkgPath string) error {
+	// Add schema present in openAPI file for current package
+	if e.NeedOpenAPI {
+		clean, err := openapi.AddSchemaFromFile(filepath.Join(pkgPath, ext.KRMFileName()))
+		if err != nil {
+			return err
+		}
+		defer clean()
+	}
+
+	if !e.SkipPkgPathPrint {
+		fmt.Fprintf(e.Writer, "%s/\n", pkgPath)
+	}
+
+	return e.CmdRunner.ExecuteCmd(e.Writer, pkgPath)
+}
+
 // ParseFieldPath parse a flag value into a field path
 func ParseFieldPath(path string) ([]string, error) {
 	// fixup '\.' so we don't split on it

kyaml/openapi/openapi.go

@@ -65,28 +65,54 @@ const SupplementaryOpenAPIFieldName = "openAPI"
 const Definitions = "definitions"
 
 // AddSchemaFromFile reads the file at path and parses the OpenAPI definitions
-// from the field "openAPI"
-func AddSchemaFromFile(path string) error {
-	return AddSchemaFromFileUsingField(path, SupplementaryOpenAPIFieldName)
-}
-
-// DeleteSchemaInFile reads the file at path and removes all the openAPI definitions
-// present in file from global schema
-func DeleteSchemaInFile(openAPIPath string) error {
-	fields, err := DefinitionRefs(openAPIPath)
+// from the field "openAPI", also returns a function to clean the added definitions
+// The returned clean function is a no-op on error, or else it's a function
+// that the caller should use to remove the added openAPI definitions from
+// global schema
+func AddSchemaFromFile(path string) (func(), error) {
+	object, err := parseOpenAPI(path)
 	if err != nil {
-		return err
+		return func() {}, err
 	}
 
-	for _, field := range fields {
-		delete(globalSchema.schema.Definitions, field)
+	defs, err := definitionRefsFromRNode(object)
+	if err != nil {
+		return func() {}, err
 	}
-	return nil
+
+	clean := func() {
+		for _, def := range defs {
+			delete(globalSchema.schema.Definitions, def)
+		}
+	}
+	return clean, addSchemaUsingField(object, SupplementaryOpenAPIFieldName)
 }
 
 // DefinitionRefs returns the list of openAPI definition references present in the
 // input openAPIPath
 func DefinitionRefs(openAPIPath string) ([]string, error) {
+	object, err := parseOpenAPI(openAPIPath)
+	if err != nil {
+		return nil, err
+	}
+	return definitionRefsFromRNode(object)
+}
+
+// definitionRefsFromRNode returns the list of openAPI definitions keys from input
+// yaml RNode
+func definitionRefsFromRNode(object *yaml.RNode) ([]string, error) {
+	definitions, err := object.Pipe(yaml.Lookup(SupplementaryOpenAPIFieldName, Definitions))
+	if definitions == nil {
+		return nil, err
+	}
+	if err != nil {
+		return nil, err
+	}
+	return definitions.Fields()
+}
+
+// parseOpenAPI reads openAPIPath yaml and converts it to RNode
+func parseOpenAPI(openAPIPath string) (*yaml.RNode, error) {
 	b, err := ioutil.ReadFile(openAPIPath)
 	if err != nil {
 		return nil, err
@@ -96,44 +122,23 @@ func DefinitionRefs(openAPIPath string) ([]string, error) {
 	if err != nil {
 		return nil, err
 	}
-
-	definitions, err := object.Pipe(yaml.Lookup(SupplementaryOpenAPIFieldName, Definitions))
-	if definitions == nil {
-		return nil, err
-	}
-	if err != nil {
-		return nil, err
-	}
-
-	return definitions.Fields()
+	return object, nil
 }
 
-// AddSchemaFromFileUsingField reads the file at path and parses the OpenAPI definitions
-// from the specified field.  If field is the empty string, use the whole document as
-// OpenAPI.
-func AddSchemaFromFileUsingField(path, field string) error {
-	b, err := ioutil.ReadFile(path)
-	if err != nil {
-		return err
-	}
-
-	// parse the yaml file (json is a subset of yaml, so will also parse)
-	y, err := yaml.Parse(string(b))
-	if err != nil {
-		return err
-	}
-
+// addSchemaUsingField parses the OpenAPI definitions from the specified field.
+// If field is the empty string, use the whole document as OpenAPI.
+func addSchemaUsingField(object *yaml.RNode, field string) error {
 	if field != "" {
 		// get the field containing the openAPI
-		m := y.Field(field)
+		m := object.Field(field)
 		if m.IsNilOrEmpty() {
 			// doesn't contain openAPI definitions
 			return nil
 		}
-		y = m.Value
+		object = m.Value
 	}
 
-	oAPI, err := y.String()
+	oAPI, err := object.String()
 	if err != nil {
 		return err
 	}

kyaml/openapi/openapi_test.go

@@ -143,10 +143,11 @@ openAPI:
 		t.FailNow()
 	}
 
-	err = AddSchemaFromFile(f.Name())
+	clean, err := AddSchemaFromFile(f.Name())
 	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
+	defer clean()
 
 	s, err := GetSchema(`{"$ref": "#/definitions/io.k8s.cli.setters.image-name"}`)
 
@@ -179,7 +180,7 @@ openAPI:
 		t.FailNow()
 	}
 
-	err = AddSchemaFromFile(f.Name())
+	clean, err := AddSchemaFromFile(f.Name())
 	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
@@ -195,10 +196,7 @@ openAPI:
 	assert.Equal(t, `map[x-k8s-cli:map[setter:map[name:image-name value:nginx]]]`,
 		fmt.Sprintf("%v", s.Schema.Extensions))
 
-	err = DeleteSchemaInFile(f.Name())
-	if !assert.NoError(t, err) {
-		t.FailNow()
-	}
+	clean()
 
 	_, err = GetSchema(`{"$ref": "#/definitions/io.k8s.cli.setters.image-name"}`)
 
@@ -225,15 +223,11 @@ openAPI:
 		t.FailNow()
 	}
 
-	err = AddSchemaFromFile(f.Name())
-	if !assert.NoError(t, err) {
-		t.FailNow()
-	}
-
-	err = DeleteSchemaInFile(f.Name())
+	clean, err := AddSchemaFromFile(f.Name())
 	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
+	defer clean()
 }
 
 func TestPopulateDefsInOpenAPI_Substitution(t *testing.T) {
@@ -271,9 +265,11 @@ openAPI:
 		t.FailNow()
 	}
 
-	if !assert.NoError(t, AddSchemaFromFile(f.Name())) {
+	clean, err := AddSchemaFromFile(f.Name())
+	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
+	defer clean()
 
 	s, err := GetSchema(`{"$ref": "#/definitions/io.k8s.cli.substitutions.image"}`)
 
@@ -305,9 +301,11 @@ kind: Example
 		t.FailNow()
 	}
 
-	if !assert.NoError(t, AddSchemaFromFile(f.Name())) {
+	clean, err := AddSchemaFromFile(f.Name())
+	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
+	defer clean()
 
 	if !assert.Equal(t, len(globalSchema.schema.Definitions), 0) {
 		t.FailNow()

kyaml/setters2/add_test.go

@@ -342,7 +342,7 @@ openAPI:
 func TestAddUpdateSubstitution(t *testing.T) {
 	path := filepath.Join(os.TempDir(), "resourcefile")
 
-	//write initial resourcefile to temp path
+	// write initial resourcefile to temp path
 	err := ioutil.WriteFile(path, []byte(resourcefile), 0666)
 	if !assert.NoError(t, err) {
 		t.FailNow()
@@ -360,7 +360,7 @@ func TestAddUpdateSubstitution(t *testing.T) {
 
 	values := []Value{value1, value2}
 
-	//add a setter definition
+	// add a setter definition
 	subd := SubstitutionDefinition{
 		Name:    "image",
 		Pattern: "IMAGE_NAME:IMAGE_TAG",

kyaml/setters2/delete_test.go

@@ -42,13 +42,13 @@ openAPI:
 func TestDelete_Filter2(t *testing.T) {
 	path := filepath.Join(os.TempDir(), "resourcefile2")
 
-	//write initial resourcefile to temp path
+	// write initial resourcefile to temp path
 	err := ioutil.WriteFile(path, []byte(resourcefile2), 0666)
 	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
 
-	//add a deleter definition
+	// add a deleter definition
 	dd := DeleterDefinition{
 		Name:             "image",
 		DefinitionPrefix: fieldmeta.SetterDefinitionPrefix,

kyaml/setters2/list.go

@@ -29,9 +29,11 @@ type List struct {
 
 // ListSetters initializes l.Setters with the setters from the OpenAPI definitions in the file
 func (l *List) ListSetters(openAPIPath, resourcePath string) error {
-	if err := openapi.AddSchemaFromFile(openAPIPath); err != nil {
+	clean, err := openapi.AddSchemaFromFile(openAPIPath)
+	if err != nil {
 		return err
 	}
+	defer clean()
 	y, err := yaml.ReadFile(openAPIPath)
 	if err != nil {
 		return err
@@ -41,9 +43,11 @@ func (l *List) ListSetters(openAPIPath, resourcePath string) error {
 
 // ListSubst initializes l.Substitutions with the substitutions from the OpenAPI definitions in the file
 func (l *List) ListSubst(openAPIPath string) error {
-	if err := openapi.AddSchemaFromFile(openAPIPath); err != nil {
+	clean, err := openapi.AddSchemaFromFile(openAPIPath)
+	if err != nil {
 		return err
 	}
+	defer clean()
 	y, err := yaml.ReadFile(openAPIPath)
 	if err != nil {
 		return err

kyaml/setters2/settersutil/deletecreator_test.go

@@ -71,10 +71,11 @@ func TestDeleterCreator_Delete(t *testing.T) {
 		DefinitionPrefix: fieldmeta.SetterDefinitionPrefix,
 	}
 
-	err = openapi.AddSchemaFromFile(openAPI.Name())
+	clean, err := openapi.AddSchemaFromFile(openAPI.Name())
 	if !assert.NoError(t, err) {
 		t.FailNow()
 	}
+	defer clean()
 	dc.OpenAPIPath = openAPI.Name()
 	dc.ResourcesPath = resource.Name()
 

kyaml/setters2/settersutil/deletercreator.go

@@ -39,9 +39,11 @@ func (d DeleterCreator) Delete() error {
 	}
 
 	// Load the updated definitions
-	if err := openapi.AddSchemaFromFile(d.OpenAPIPath); err != nil {
+	clean, err := openapi.AddSchemaFromFile(d.OpenAPIPath)
+	if err != nil {
 		return err
 	}
+	defer clean()
 
 	// Update the resources with the deleter reference
 	inout := &kio.LocalPackageReadWriter{PackagePath: d.ResourcesPath, PackageFileName: d.OpenAPIFileName}

kyaml/setters2/settersutil/fieldsetter.go

@@ -79,9 +79,11 @@ func (fs FieldSetter) Set() (int, error) {
 	}
 
 	// Load the updated definitions
-	if err := openapi.AddSchemaFromFile(fs.OpenAPIPath); err != nil {
+	clean, err := openapi.AddSchemaFromFile(fs.OpenAPIPath)
+	if err != nil {
 		return 0, err
 	}
+	defer clean()
 
 	// Update the resources with the new value
 	// Set NoDeleteFiles to true as SetAll will return only the nodes of files which should be updated and
@@ -108,9 +110,11 @@ func (fs FieldSetter) Set() (int, error) {
 // If syncOpenAPI is true, the openAPI files in destination directories are also
 // updated with the setter values in the input openAPI file
 func SetAllSetterDefinitions(syncOpenAPI bool, openAPIPath string, dirs ...string) error {
-	if err := openapi.AddSchemaFromFile(openAPIPath); err != nil {
+	clean, err := openapi.AddSchemaFromFile(openAPIPath)
+	if err != nil {
 		return err
 	}
+	defer clean()
 	for _, destDir := range dirs {
 		if syncOpenAPI {
 			openAPIFileName := filepath.Base(openAPIPath)
@@ -122,7 +126,7 @@ func SetAllSetterDefinitions(syncOpenAPI bool, openAPIPath string, dirs ...strin
 		rw := &kio.LocalPackageReadWriter{
 			PackagePath: destDir,
 			// set output won't include resources from files which
-			//weren't modified.  make sure we don't delete them.
+			// weren't modified.  make sure we don't delete them.
 			NoDeleteFiles: true,
 		}
 

kyaml/setters2/settersutil/settercreator.go

@@ -103,9 +103,11 @@ func (c SetterCreator) Create() error {
 	}
 
 	// Load the updated definitions
-	if err := openapi.AddSchemaFromFile(c.OpenAPIPath); err != nil {
+	clean, err := openapi.AddSchemaFromFile(c.OpenAPIPath)
+	if err != nil {
 		return err
 	}
+	defer clean()
 	// if the setter is of array type write the derived list values back to
 	// openAPI definitions
 	if len(a.ListValues) > 0 {

kyaml/setters2/settersutil/substitutioncreator.go

@@ -93,9 +93,11 @@ func (c SubstitutionCreator) Create() error {
 	}
 
 	// Load the updated definitions
-	if err := openapi.AddSchemaFromFile(c.OpenAPIPath); err != nil {
+	clean, err := openapi.AddSchemaFromFile(c.OpenAPIPath)
+	if err != nil {
 		return err
 	}
+	defer clean()
 
 	visited := sets.String{}
 	ref, err := spec.NewRef(fieldmeta.DefinitionsPrefix + fieldmeta.SubstitutionDefinitionPrefix + c.Name)
@@ -119,9 +121,11 @@ func (c SubstitutionCreator) Create() error {
 	}
 
 	// Load the updated definitions after setters are created
-	if err := openapi.AddSchemaFromFile(c.OpenAPIPath); err != nil {
+	clean, err = openapi.AddSchemaFromFile(c.OpenAPIPath)
+	if err != nil {
 		return err
 	}
+	defer clean()
 
 	// revert openAPI file if there are cycles detected in created input substitution
 	if err := checkForCycles(ext, visited); err != nil {

kyaml/setters2/settersutil/substitutioncreator_test.go

@@ -103,7 +103,7 @@ func TestGetValuesForMarkers(t *testing.T) {
 					}
 				}
 			} else {
-				//if expectedError is not nil, check for correctness of error message
+				// if expectedError is not nil, check for correctness of error message
 				assert.Contains(t, err.Error(), test.expectedError.Error())
 			}
 		})

kyaml/setters2/util_test.go

@@ -154,7 +154,8 @@ openAPI:
 			if !assert.NoError(t, err) {
 				t.FailNow()
 			}
-			err = openapi.AddSchemaFromFile(filepath.Join(dir, "Krmfile"))
+			clean, err := openapi.AddSchemaFromFile(filepath.Join(dir, "Krmfile"))
+			defer clean()
 			if err != nil {
 				// do nothing if openAPI file or schema doesn't exist, CheckRequiredSettersSet()
 				// should not throw any error