refactor: improve error messages for brick update and validation processes

Author: dido18

internal/orchestrator/bricks/bricks.go

@@ -282,13 +282,13 @@ func (s *Service) BrickCreate(
 		if !exist {
 			return fmt.Errorf("variable %q does not exist on brick %q", name, brick.ID)
 		}
-		if value.DefaultValue == "" && reqValue == "" {
-			return fmt.Errorf("variable %q cannot be empty", name)
+		if value.IsRequired() && reqValue == "" {
+			return fmt.Errorf("requried variable %q cannot be empty", name)
 		}
 	}
 
 	for _, brickVar := range brick.Variables {
-		if brickVar.DefaultValue == "" {
+		if brickVar.IsRequired() {
 			if _, exist := req.Variables[brickVar.Name]; !exist {
 				slog.Warn("[Skip] a required variable is not set by user", "variable", brickVar.Name, "brick", brickVar.Name)
 			}
@@ -335,9 +335,13 @@ func (s *Service) BrickUpdate(
 	req BrickCreateUpdateRequest,
 	appCurrent app.ArduinoApp,
 ) error {
+	_, present := s.bricksIndex.FindBrickByID(req.ID)
+	if !present {
+		return fmt.Errorf("brick %q not found into the brick index", req.ID)
+	}
 	index := slices.IndexFunc(appCurrent.Descriptor.Bricks, func(b app.Brick) bool { return b.ID == req.ID })
 	if index == -1 {
-		return fmt.Errorf("brick not found with id %s", req.ID)
+		return fmt.Errorf("brick %q not found into the bricks of the app", req.ID)
 	}
 	brickID := appCurrent.Descriptor.Bricks[index].ID
 	brickVariables := appCurrent.Descriptor.Bricks[index].Variables
@@ -356,15 +360,16 @@ func (s *Service) BrickUpdate(
 	}
 	brick, present := s.bricksIndex.FindBrickByID(brickID)
 	if !present {
-		return fmt.Errorf("brick not found with id %s", brickID)
+		return fmt.Errorf("brick %q not found in the brick index", brickID)
 	}
+
 	for name, updateValue := range req.Variables {
 		value, exist := brick.GetVariable(name)
 		if !exist {
-			return errors.New("variable does not exist")
+			return fmt.Errorf("variable %q does not exist on brick %q", name, brick.ID)
 		}
-		if value.DefaultValue == "" && updateValue == "" {
-			return errors.New("variable default value cannot be empty")
+		if value.IsRequired() && updateValue == "" {
+			return fmt.Errorf("required variable %q cannot be empty", name)
 		}
 		updated := false
 		for _, v := range brickVariables {
@@ -374,12 +379,19 @@ func (s *Service) BrickUpdate(
 				break
 			}
 		}
-
 		if !updated {
 			brickVariables[name] = updateValue
 		}
 	}
 
+	for _, brickVar := range brick.Variables {
+		if brickVar.IsRequired() {
+			if _, exist := req.Variables[brickVar.Name]; !exist {
+				return fmt.Errorf("required variable %q must be set", brickVar.Name)
+			}
+		}
+	}
+
 	appCurrent.Descriptor.Bricks[index].Model = brickModel
 	appCurrent.Descriptor.Bricks[index].Variables = brickVariables
 

internal/orchestrator/bricks/bricks_test.go

@@ -26,6 +26,86 @@ import (
 	"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
 )
 
+func TestUdateBrick(t *testing.T) {
+	bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
+	require.Nil(t, err)
+	brickService := NewService(nil, bricksIndex, nil)
+
+	t.Run("fails if brick id does not exist into brick index", func(t *testing.T) {
+		err = brickService.BrickUpdate(BrickCreateUpdateRequest{ID: "not-existing-id"}, f.Must(app.Load("testdata/dummy-app")))
+		require.Error(t, err)
+		require.Equal(t, "brick \"not-existing-id\" not found into the brick index", err.Error())
+	})
+
+	t.Run("fails if brick id 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)
+		require.Equal(t, "brick \"not-existing-id\" not found into app descriptor", err.Error())
+	})
+
+	t.Run("fails if the updated variable is not present in the brick definition", func(t *testing.T) {
+		req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{
+			"NON_EXISTING_VARIABLE": "some-value",
+		}}
+		err = brickService.BrickUpdate(req, f.Must(app.Load("testdata/dummy-app")))
+		require.Error(t, err)
+		require.Equal(t, "variable \"NON_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\"", err.Error())
+	})
+
+	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": "",
+			"ARDUINO_SECRET":    "a-secret-a",
+		}}
+		err = brickService.BrickUpdate(req, f.Must(app.Load("testdata/dummy-app")))
+		require.Error(t, err)
+		require.Equal(t, "required variable \"ARDUINO_DEVICE_ID\" cannot be empty", err.Error())
+	})
+
+	t.Run("fails if a mandatory variable is not present", func(t *testing.T) {
+		tempDummyApp := paths.New("testdata/dummy-app.temp")
+		err := tempDummyApp.RemoveAll()
+		require.Nil(t, err)
+		require.Nil(t, paths.New("testdata/dummy-app").CopyDirTo(tempDummyApp))
+
+		req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{
+			"ARDUINO_SECRET": "a-secret-a",
+		}}
+		err = brickService.BrickUpdate(req, f.Must(app.Load(tempDummyApp.String())))
+		require.Error(t, err)
+		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")
+		require.Nil(t, tempDummyApp.RemoveAll())
+		require.Nil(t, paths.New("testdata/dummy-app").CopyDirTo(tempDummyApp))
+		bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
+		require.Nil(t, err)
+		brickService := NewService(nil, bricksIndex, nil)
+
+		deviceID := "updated-device-id"
+		secret := "updated-secret"
+		req := BrickCreateUpdateRequest{
+			ID: "arduino:arduino_cloud",
+			Variables: map[string]string{
+				"ARDUINO_DEVICE_ID": deviceID,
+				"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, deviceID, after.Descriptor.Bricks[0].Variables["ARDUINO_DEVICE_ID"])
+		require.Equal(t, secret, after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"])
+	})
+
+}
 func TestBrickCreate(t *testing.T) {
 	bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
 	require.Nil(t, err)
@@ -90,6 +170,7 @@ func TestBrickCreate(t *testing.T) {
 		require.Len(t, after.Descriptor.Bricks, 2)
 		require.Equal(t, "arduino:dbstorage_sqlstore", after.Descriptor.Bricks[1].ID)
 	})
+
 	t.Run("the variables of a brick are updated", func(t *testing.T) {
 		tempDummyApp := paths.New("testdata/dummy-app.brick-override.temp")
 		err := tempDummyApp.RemoveAll()

internal/orchestrator/orchestrator.go

@@ -452,7 +452,6 @@ func RestartApp(
 	return func(yield func(StreamMessage) bool) {
 		ctx, cancel := context.WithCancel(ctx)
 		defer cancel()
-
 		runningApp, err := getRunningApp(ctx, docker.Client())
 		if err != nil {
 			yield(StreamMessage{error: err})