fix(api): allow missing required variable when a brick is added

internal/api/handlers/bricks.go

@@ -146,7 +146,7 @@ func HandleBrickCreate(
 		err = brickService.BrickCreate(req, app)
 		if err != nil {
 			// TODO: handle specific errors
-			slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()))
+			slog.Error("Unable to create brick", slog.String("error", err.Error()))
 			render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "error while creating or updating brick"})
 			return
 		}

internal/orchestrator/app/testdata/validator/bricks-list.yaml

@@ -0,0 +1,9 @@
+bricks:
+- id: arduino:arduino_cloud
+  name: Arduino Cloud
+  description: Connects to Arduino Cloud
+  variables:
+  - name: ARDUINO_DEVICE_ID
+    description: Arduino Cloud Device ID
+  - name: ARDUINO_SECRET
+    description: Arduino Cloud Secret

internal/orchestrator/app/testdata/validator/empty-bricks-app.yaml

@@ -0,0 +1,4 @@
+name: App with empty bricks
+description: App with empty bricks
+
+bricks: []

internal/orchestrator/app/testdata/validator/missing-required-app.yaml

@@ -0,0 +1,4 @@
+name: App with no required variables
+description: App with no required variables
+bricks:
+  - arduino:arduino_cloud

internal/orchestrator/app/testdata/validator/mixed-required-app.yaml

@@ -0,0 +1,6 @@
+name: App only one required variable filled
+description: App only one required variable filled
+bricks:
+  - arduino:arduino_cloud:
+      variables:
+        ARDUINO_DEVICE_ID: "my-device-id"

internal/orchestrator/app/testdata/validator/no-bricks-app.yaml

@@ -0,0 +1,2 @@
+name: App with no bricks
+description: App with no bricks description

internal/orchestrator/app/testdata/validator/not-found-brick-app.yaml

@@ -0,0 +1,4 @@
+name: App no existing brick
+description: App no existing brick
+bricks:
+  - arduino:not_existing_brick

internal/orchestrator/app/validator.go

@@ -0,0 +1,38 @@
+package app
+
+import (
+	"fmt"
+
+	"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
+)
+
+func ValidateAppDescriptor(a AppDescriptor, index *bricksindex.BricksIndex) error {
+	return validatebricks(a, index)
+}
+
+func validatebricks(a AppDescriptor, index *bricksindex.BricksIndex) error {
+	for _, appBrick := range a.Bricks {
+		indexBrick, found := index.FindBrickByID(appBrick.ID)
+		if !found {
+			return fmt.Errorf("brick %q not found", appBrick.ID)
+		}
+
+		// check the bricks variables inside the app.yaml are valid given a brick definition
+		for appBrickName := range appBrick.Variables {
+			_, exist := indexBrick.GetVariable(appBrickName)
+			if !exist {
+				return fmt.Errorf("variable %q does not exist on brick %q", appBrickName, indexBrick.ID)
+			}
+		}
+
+		// check all required variables has a value
+		for _, indexBrickVariable := range indexBrick.Variables {
+			if indexBrickVariable.IsRequired() {
+				if _, exist := appBrick.Variables[indexBrickVariable.Name]; !exist {
+					return fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID)
+				}
+			}
+		}
+	}
+	return nil
+}

internal/orchestrator/app/validator_test.go

@@ -0,0 +1,53 @@
+package app
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/arduino/go-paths-helper"
+
+	"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
+)
+
+func TestValidateAppDescriptor(t *testing.T) {
+	bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata/validator"))
+	require.Nil(t, err)
+
+	t.Run("valid app descriptor with no bricks", func(t *testing.T) {
+		app, err := ParseDescriptorFile(paths.New("testdata/validator/no-bricks-app.yaml"))
+		require.NoError(t, err)
+		err = ValidateAppDescriptor(app, bricksIndex)
+		assert.NoError(t, err)
+	})
+
+	t.Run("valid app descriptor with empty list of bricks", func(t *testing.T) {
+		app, err := ParseDescriptorFile(paths.New("testdata/validator/empty-bricks-app.yaml"))
+		require.NoError(t, err)
+		err = ValidateAppDescriptor(app, bricksIndex)
+		assert.NoError(t, err)
+	})
+
+	t.Run("invalid app descriptor with missing required variable", func(t *testing.T) {
+		app, err := ParseDescriptorFile(paths.New("testdata/validator/missing-required-app.yaml"))
+		require.NoError(t, err)
+		err = ValidateAppDescriptor(app, bricksIndex)
+		assert.Equal(t, "variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\"", err.Error())
+	})
+
+	t.Run("invalid app descriptor with a missing required variable among two", func(t *testing.T) {
+		app, err := ParseDescriptorFile(paths.New("testdata/validator/mixed-required-app.yaml"))
+		require.NoError(t, err)
+		err = ValidateAppDescriptor(app, bricksIndex)
+		assert.Equal(t, "variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\"", err.Error())
+	})
+
+	t.Run("invalid app descriptor with not found brick id", func(t *testing.T) {
+		app, err := ParseDescriptorFile(paths.New("testdata/validator/not-found-brick-app.yaml"))
+		require.NoError(t, err)
+		err = ValidateAppDescriptor(app, bricksIndex)
+		assert.Equal(t, "brick \"arduino:not_existing_brick\" not found", err.Error())
+	})
+
+}

internal/orchestrator/bricks/bricks.go

@@ -290,7 +290,7 @@ func (s *Service) BrickCreate(
 	for _, brickVar := range brick.Variables {
 		if brickVar.DefaultValue == "" {
 			if _, exist := req.Variables[brickVar.Name]; !exist {
-				return fmt.Errorf("required variable %q is mandatory", brickVar.Name)
+				slog.Warn("[Skip] a required variable is not set by user", "variable", brickVar.Name)
 			}
 		}
 	}

internal/orchestrator/bricks/bricks_test.go

@@ -16,7 +16,9 @@
 package bricks
 
 import (
+	"fmt"
 	"testing"
+	"time"
 
 	"github.com/arduino/go-paths-helper"
 	"github.com/stretchr/testify/require"
@@ -32,7 +34,7 @@ func TestBrickCreate(t *testing.T) {
 	brickService := NewService(nil, bricksIndex, nil)
 
 	t.Run("fails if brick id does not exist", func(t *testing.T) {
-		err = brickService.BrickCreate(BrickCreateUpdateRequest{ID: "not-existing-id"}, f.Must(app.Load("testdata/dummy-app")))
+		err = brickService.BrickCreate(BrickCreateUpdateRequest{ID: "not-existing-id"}, f.Must(app.Load("testdata/AppFromExample")))
 		require.Error(t, err)
 		require.Equal(t, "brick \"not-existing-id\" not found", err.Error())
 	})
@@ -41,50 +43,59 @@ func TestBrickCreate(t *testing.T) {
 		req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{
 			"NON_EXISTING_VARIABLE": "some-value",
 		}}
-		err = brickService.BrickCreate(req, f.Must(app.Load("testdata/dummy-app")))
+		err = brickService.BrickCreate(req, f.Must(app.Load("testdata/AppFromExample")))
 		require.Error(t, err)
 		require.Equal(t, "variable \"NON_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\"", err.Error())
 	})
 
+	//TODO: currently we do not accept an empty string as a valid value for a variable
 	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.BrickCreate(req, f.Must(app.Load("testdata/dummy-app")))
+		err = brickService.BrickCreate(req, f.Must(app.Load("testdata/AppFromExample")))
 		require.Error(t, err)
 		require.Equal(t, "variable \"ARDUINO_DEVICE_ID\" cannot be empty", err.Error())
 	})
 
-	t.Run("fails if a mandatory variable is not present in the request", func(t *testing.T) {
+	t.Run("omit a mandatory variable is not present in the request", func(t *testing.T) {
+		tempApp, cleanUp := copyToTempApp(t, paths.New("testdata/AppFromExample"))
+		defer cleanUp()
+
 		req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{
 			"ARDUINO_SECRET": "a-secret-a",
 		}}
-		err = brickService.BrickCreate(req, f.Must(app.Load("testdata/dummy-app")))
-		require.Error(t, err)
-		require.Equal(t, "required variable \"ARDUINO_DEVICE_ID\" is mandatory", err.Error())
+		err = brickService.BrickCreate(req, f.Must(app.Load(tempApp.String())))
+		require.Nil(t, err)
+
+		after, err := app.Load(tempApp.String())
+		require.Nil(t, err)
+		require.Len(t, after.Descriptor.Bricks, 1)
+		require.Equal(t, "arduino:arduino_cloud", after.Descriptor.Bricks[0].ID)
+		// NOTE: currently it is not possible to distinguish a field with empty string or missing field into the yaml.
+		// The 'ARDUINO_DEVICE_ID' is missing from the app.yaml but here we check the empty string.
+		// A better aproach is to use golden files
+		require.Equal(t, "", after.Descriptor.Bricks[0].Variables["ARDUINO_DEVICE_ID"])
+		require.Equal(t, "a-secret-a", after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"])
 	})
 
 	t.Run("the brick is added if it does not exist in the app", 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))
+		tempApp, cleanUp := copyToTempApp(t, paths.New("testdata/AppFromExample"))
+		defer cleanUp()
 
 		req := BrickCreateUpdateRequest{ID: "arduino:dbstorage_sqlstore"}
-		err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String())))
+		err = brickService.BrickCreate(req, f.Must(app.Load(tempApp.String())))
 		require.Nil(t, err)
-		after, err := app.Load(tempDummyApp.String())
+		after, err := app.Load(tempApp.String())
 		require.Nil(t, err)
 		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()
-		require.Nil(t, err)
-		err = paths.New("testdata/dummy-app").CopyDirTo(tempDummyApp)
-		require.Nil(t, err)
+		tempApp, cleanUp := copyToTempApp(t, paths.New("testdata/AppFromExample"))
+		defer cleanUp()
+
 		bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
 		require.Nil(t, err)
 		brickService := NewService(nil, bricksIndex, nil)
@@ -99,10 +110,10 @@ func TestBrickCreate(t *testing.T) {
 			},
 		}
 
-		err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String())))
+		err = brickService.BrickCreate(req, f.Must(app.Load(tempApp.String())))
 		require.Nil(t, err)
 
-		after, err := app.Load(tempDummyApp.String())
+		after, err := app.Load(tempApp.String())
 		require.Nil(t, err)
 		require.Len(t, after.Descriptor.Bricks, 1)
 		require.Equal(t, "arduino:arduino_cloud", after.Descriptor.Bricks[0].ID)
@@ -111,6 +122,14 @@ func TestBrickCreate(t *testing.T) {
 	})
 }
 
+func copyToTempApp(t *testing.T, srcApp *paths.Path) (tmpApp *paths.Path, cleanUp func()) {
+	tmpAppPath := paths.New(srcApp.String() + "-" + fmt.Sprint(time.Now().UnixMicro()) + ".temp")
+	require.Nil(t, srcApp.CopyDirTo(tmpAppPath))
+	return tmpAppPath, func() {
+		require.Nil(t, tmpAppPath.RemoveAll())
+	}
+}
+
 func TestGetBrickInstanceVariableDetails(t *testing.T) {
 	tests := []struct {
 		name                    string