refactor: update brick handling and improve test coverage for variable updates

Author: dido18

internal/orchestrator/bricks/bricks.go

@@ -381,17 +381,18 @@ func (s *Service) BrickUpdate(
 		}
 	}
 
+	appCurrent.Descriptor.Bricks[brickPosition].Model = brickModel
+	appCurrent.Descriptor.Bricks[brickPosition].Variables = brickVariables
+
 	for _, brickVar := range brickFromIndex.Variables {
 		if brickVar.IsRequired() {
-			if _, exist := req.Variables[brickVar.Name]; !exist {
+			newVariables := appCurrent.Descriptor.Bricks[brickPosition].Variables
+			if _, exist := newVariables[brickVar.Name]; !exist {
 				return fmt.Errorf("required variable %q must be set", brickVar.Name)
 			}
 		}
 	}
 
-	appCurrent.Descriptor.Bricks[brickPosition].Model = brickModel
-	appCurrent.Descriptor.Bricks[brickPosition].Variables = brickVariables
-
 	err := appCurrent.Save()
 	if err != nil {
 		return fmt.Errorf("cannot save brick instance with id %s", req.ID)

internal/orchestrator/bricks/bricks_test.go

@@ -134,7 +134,6 @@ func TestUdateBrick(t *testing.T) {
 		require.Equal(t, "brick \"not-existing-id\" not found into the brick index", err.Error())
 	})
 
-	// TODO: if a brick is updated but it is corrunlty not in the app.yaml ? ithink we must add it to the app.yaml
 	t.Run("fails if brick is present into the index but not in the app ", func(t *testing.T) {
 		err = brickService.BrickUpdate(BrickCreateUpdateRequest{ID: "arduino:dbstorage_sqlstore"}, f.Must(app.Load("testdata/dummy-app")))
 		require.Error(t, err)
@@ -150,6 +149,7 @@ func TestUdateBrick(t *testing.T) {
 		require.Equal(t, "variable \"NON_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\"", err.Error())
 	})
 
+	// TODO: allow to set an empty "" varaible
 	t.Run("fails if a required variable is set empty", func(t *testing.T) {
 		req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{
 			"ARDUINO_DEVICE_ID": "",
@@ -174,8 +174,8 @@ func TestUdateBrick(t *testing.T) {
 		require.Equal(t, "required variable \"ARDUINO_DEVICE_ID\" must be set", err.Error())
 	})
 
-	t.Run("update the variables of a brick correctly", func(t *testing.T) {
-		tempDummyApp := paths.New("testdata/dummy-app.brick-override.temp")
+	t.Run("update a single variables of a brick correctly", func(t *testing.T) {
+		tempDummyApp := paths.New("testdata/dummy-app.temp")
 		require.Nil(t, tempDummyApp.RemoveAll())
 		require.Nil(t, paths.New("testdata/dummy-app").CopyDirTo(tempDummyApp))
 		bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
@@ -203,6 +203,34 @@ func TestUdateBrick(t *testing.T) {
 		require.Equal(t, secret, after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"])
 	})
 
+	t.Run("update a single variable correctly", func(t *testing.T) {
+		tempDummyApp := paths.New("testdata/dummy-app-for-update.temp")
+		require.Nil(t, tempDummyApp.RemoveAll())
+		require.Nil(t, paths.New("testdata/dummy-app-for-update").CopyDirTo(tempDummyApp))
+		bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
+		require.Nil(t, err)
+		brickService := NewService(nil, bricksIndex, nil)
+
+		secret := "updated-the-secret"
+		req := BrickCreateUpdateRequest{
+			ID: "arduino:arduino_cloud",
+			Variables: map[string]string{
+				// the ARDUINO_DEVICE_ID is already configured int the app.yaml
+				"ARDUINO_SECRET": secret,
+			},
+		}
+
+		err = brickService.BrickUpdate(req, f.Must(app.Load(tempDummyApp.String())))
+		require.Nil(t, err)
+
+		after, err := app.Load(tempDummyApp.String())
+		require.Nil(t, err)
+		require.Len(t, after.Descriptor.Bricks, 1)
+		require.Equal(t, "arduino:arduino_cloud", after.Descriptor.Bricks[0].ID)
+		require.Equal(t, "i-am-a-device-id", after.Descriptor.Bricks[0].Variables["ARDUINO_DEVICE_ID"])
+		require.Equal(t, secret, after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"])
+	})
+
 }
 
 func TestGetBrickInstanceVariableDetails(t *testing.T) {

internal/orchestrator/bricks/testdata/dummy-app-for-update/app.yaml

@@ -0,0 +1,6 @@
+name: An app with only a required variable filled
+description: An app with only a required variable filled
+bricks:
+  - arduino:arduino_cloud:
+      variables:
+        ARDUINO_DEVICE_ID: "i-am-a-device-id"

internal/orchestrator/bricks/testdata/dummy-app-for-update/python/main.py

@@ -0,0 +1,2 @@
+def main():
+    pass