Address PR review feedback - attribute funcs

Signed-off-by: Maysun J Faisal <[email protected]>

Author: maysunfaisal

pkg/devfile/parser/data/interface.go

@@ -21,7 +21,7 @@ type DevfileData interface {
 
 	GetAttributes() (attributes.Attributes, error)
 	AddAttributes(key string, value interface{}) error
-	UpdateAttributes(attr attributes.Attributes) error
+	UpdateAttributes(key string, value interface{}) error
 
 	// parent related methods
 

pkg/devfile/parser/data/v2/attributes.go

@@ -17,32 +17,36 @@ func (d *DevfileV2) GetAttributes() (attributes.Attributes, error) {
 	}
 }
 
-// UpdateAttributes updates the devfile top level attributes
-func (d *DevfileV2) UpdateAttributes(attr attributes.Attributes) error {
+// UpdateAttributes updates the devfile top level attribute for the specific key, err out if key is absent
+func (d *DevfileV2) UpdateAttributes(key string, value interface{}) error {
+	var err error
+
 	// This feature was introduced in 2.1.0; so any version 2.1.0 and up should use the 2.1.0 implementation
 	switch d.SchemaVersion {
 	case "2.0.0":
 		return fmt.Errorf("top-level attributes is not supported in devfile schema version 2.0.0")
 	default:
-		d.Attributes = attr
+		if d.Attributes.Exists(key) {
+			d.Attributes.Put(key, value, &err)
+		} else {
+			return fmt.Errorf("cannot update top-level attribute, key %s is not present", key)
+		}
 	}
 
-	return nil
+	return err
 }
 
-// AddAttributes adds to the devfile top level attributes
+// AddAttributes adds to the devfile top level attributes, value will be overwritten if key is already present
 func (d *DevfileV2) AddAttributes(key string, value interface{}) error {
+	var err error
+
 	// This feature was introduced in 2.1.0; so any version 2.1.0 and up should use the 2.1.0 implementation
 	switch d.SchemaVersion {
 	case "2.0.0":
 		return fmt.Errorf("top-level attributes is not supported in devfile schema version 2.0.0")
 	default:
-		var err error
 		d.Attributes.Put(key, value, &err)
-		if err != nil {
-			return err
-		}
 	}
 
-	return nil
+	return err
 }

pkg/devfile/parser/data/v2/attributes_test.go

@@ -62,11 +62,19 @@ func TestGetAttributes(t *testing.T) {
 
 func TestUpdateAttributes(t *testing.T) {
 
+	nestedValue := map[string]interface{}{
+		"key1.1": map[string]interface{}{
+			"key1.1.1": "value1.1.1",
+		},
+	}
+
 	tests := []struct {
-		name       string
-		devfilev2  *DevfileV2
-		attributes attributes.Attributes
-		wantErr    bool
+		name           string
+		devfilev2      *DevfileV2
+		key            string
+		value          interface{}
+		wantAttributes attributes.Attributes
+		wantErr        bool
 	}{
 		{
 			name: "Schema 2.0.0 does not have attributes",
@@ -80,7 +88,25 @@ func TestUpdateAttributes(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "Schema 2.1.0 has attributes",
+			name: "Schema 2.1.0 has the top-level key attribute",
+			devfilev2: &DevfileV2{
+				v1alpha2.Devfile{
+					DevfileHeader: devfilepkg.DevfileHeader{
+						SchemaVersion: "2.1.0",
+					},
+					DevWorkspaceTemplateSpec: v1alpha2.DevWorkspaceTemplateSpec{
+						DevWorkspaceTemplateSpecContent: v1alpha2.DevWorkspaceTemplateSpecContent{
+							Attributes: attributes.Attributes{}.PutString("key1", "value1").PutString("key2", "value2"),
+						},
+					},
+				},
+			},
+			key:            "key1",
+			value:          nestedValue,
+			wantAttributes: attributes.Attributes{}.Put("key1", nestedValue, nil).PutString("key2", "value2"),
+		},
+		{
+			name: "Schema 2.1.0 does not have the top-level key attribute",
 			devfilev2: &DevfileV2{
 				v1alpha2.Devfile{
 					DevfileHeader: devfilepkg.DevfileHeader{
@@ -93,21 +119,24 @@ func TestUpdateAttributes(t *testing.T) {
 					},
 				},
 			},
-			attributes: attributes.Attributes{}.PutString("key3", "value3"),
+			key:     "key_invalid",
+			value:   nestedValue,
+			wantErr: true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := tt.devfilev2.UpdateAttributes(tt.attributes)
+			err := tt.devfilev2.UpdateAttributes(tt.key, tt.value)
 			if tt.wantErr == (err == nil) {
 				t.Errorf("TestUpdateAttributes error - %v, wantErr %v", err, tt.wantErr)
 			} else if err == nil {
 				attributes, err := tt.devfilev2.GetAttributes()
 				if err != nil {
-					t.Errorf("TestUpdateAttributes error2 - %+v", err)
+					t.Errorf("TestUpdateAttributes error - %+v", err)
+					return
 				}
-				if !reflect.DeepEqual(attributes, tt.attributes) {
-					t.Errorf("TestUpdateAttributes mismatch error - expected %+v, actual %+v", tt.attributes, attributes)
+				if !reflect.DeepEqual(attributes, tt.wantAttributes) {
+					t.Errorf("TestUpdateAttributes mismatch error - expected %+v, actual %+v", tt.wantAttributes, attributes)
 				}
 			}
 		})
@@ -186,7 +215,8 @@ func TestAddAttributes(t *testing.T) {
 			} else if err == nil {
 				attributes, err := tt.devfilev2.GetAttributes()
 				if err != nil {
-					t.Errorf("TestAddAttributes error2 - %+v", err)
+					t.Errorf("TestAddAttributes error - %+v", err)
+					return
 				}
 				if !reflect.DeepEqual(attributes, tt.wantAttributes) {
 					t.Errorf("TestAddAttributes mismatch error - expected %+v, actual %+v", tt.wantAttributes, attributes)