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

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/all-required-app.yaml

@@ -0,0 +1,7 @@
+name: App ok
+description: App ok
+bricks:
+  - arduino:arduino_cloud:
+      variables:
+        ARDUINO_DEVICE_ID: "my-device-id"
+        ARDUINO_SECRET: "my-secret"

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/empty-required-app.yaml

@@ -0,0 +1,7 @@
+name: App with an empty variable
+description: App withan empty variable
+bricks:
+  - arduino:arduino_cloud:
+      variables:
+        ARDUINO_DEVICE_ID: "my-device-id"
+        ARDUINO_SECRET:

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,7 @@
+name: App no existing brick
+description: App no existing brick
+bricks:
+  - arduino:not_existing_brick:
+      variables:
+        ARDUINO_DEVICE_ID: "my-device-id"
+        ARDUINO_SECRET: "LAKDJ"

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

@@ -0,0 +1,8 @@
+name: App with non existing variable
+description: App with non existing variable
+bricks:
+  - arduino:arduino_cloud:
+      variables:
+        NOT_EXISTING_VARIABLE: "this-is-a-not-existing-variable-for-the-brick"
+        ARDUINO_DEVICE_ID: "my-device-id"
+        ARDUINO_SECRET: "my-secret"

internal/orchestrator/app/testdata/validator/omitted-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/omitted-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/validator.go

@@ -0,0 +1,45 @@
+package app
+
+import (
+	"errors"
+	"fmt"
+	"log/slog"
+
+	"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
+)
+
+// ValidateBricks checks that all bricks referenced in the given AppDescriptor exist in the provided BricksIndex,
+// It collects and returns all validatio errors as a single joined error, allowing the caller to see all issues at once rather than stopping at the first error.
+// If the index or the app is nil, validation is skipped and nil is returned.
+func ValidateBricks(a AppDescriptor, index *bricksindex.BricksIndex) error {
+	if index == nil {
+		return nil
+	}
+
+	var allErrors error
+
+	for _, appBrick := range a.Bricks {
+		indexBrick, found := index.FindBrickByID(appBrick.ID)
+		if !found {
+			allErrors = errors.Join(allErrors, fmt.Errorf("brick %q not found", appBrick.ID))
+			continue // Skip further validation for this brick since it doesn't exist
+		}
+
+		for appBrickName := range appBrick.Variables {
+			_, exist := indexBrick.GetVariable(appBrickName)
+			if !exist {
+				slog.Warn("variable is referenced but not declared in the brick configuration", "variable", appBrickName, "brick", indexBrick.ID)
+			}
+		}
+
+		// Check that all required brick variables are provided by app
+		for _, indexBrickVariable := range indexBrick.Variables {
+			if indexBrickVariable.IsRequired() {
+				if _, exist := appBrick.Variables[indexBrickVariable.Name]; !exist {
+					allErrors = errors.Join(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID))
+				}
+			}
+		}
+	}
+	return allErrors
+}

internal/orchestrator/app/validator_test.go

@@ -0,0 +1,84 @@
+package app
+
+import (
+	"errors"
+	"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 TestValidateAppDescriptorBricks(t *testing.T) {
+	bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata/validator"))
+	require.Nil(t, err)
+	require.NotNil(t, bricksIndex)
+
+	testCases := []struct {
+		name          string
+		filename      string
+		expectedError error
+	}{
+		{
+			name:          "valid with all required filled",
+			filename:      "all-required-app.yaml",
+			expectedError: nil,
+		},
+		{
+			name:          "valid with missing bricks",
+			filename:      "no-bricks-app.yaml",
+			expectedError: nil,
+		},
+		{
+			name:          "valid with empty list of bricks",
+			filename:      "empty-bricks-app.yaml",
+			expectedError: nil,
+		},
+		{
+			name:          "valid if required variable is empty string",
+			filename:      "empty-required-app.yaml",
+			expectedError: nil,
+		},
+		{
+			name:     "invalid if required variable is omitted",
+			filename: "omitted-required-app.yaml",
+			expectedError: errors.Join(
+				errors.New("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\""),
+				errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
+			),
+		},
+		{
+			name:          "invalid if a required variable among two is omitted",
+			filename:      "omitted-mixed-required-app.yaml",
+			expectedError: errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
+		},
+		{
+			name:          "invalid if brick id not found",
+			filename:      "not-found-brick-app.yaml",
+			expectedError: errors.New("brick \"arduino:not_existing_brick\" not found"),
+		},
+		{
+			name:          "log a warning if variable does not exist in the brick",
+			filename:      "not-found-variable-app.yaml",
+			expectedError: nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			appDescriptor, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename))
+			require.NoError(t, err)
+
+			err = ValidateBricks(appDescriptor, bricksIndex)
+			if tc.expectedError == nil {
+				assert.NoError(t, err, "Expected no validation errors")
+			} else {
+				require.Error(t, err, "Expected validation error")
+				assert.Equal(t, tc.expectedError.Error(), err.Error(), "Error message should match")
+			}
+		})
+	}
+}

internal/orchestrator/bricks/bricks.go

@@ -290,7 +290,8 @@ 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)
+				// See issue https://github.com/arduino/arduino-app-cli/issues/68
+				slog.Warn("[Skip] a required variable is not set by user", "variable", brickVar.Name)
 			}
 		}
 	}

internal/orchestrator/bricks/bricks_test.go

@@ -56,13 +56,24 @@ func TestBrickCreate(t *testing.T) {
 		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("do not fail 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.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(tempDummyApp.String())))
+		require.NoError(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, "", 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) {

internal/orchestrator/orchestrator.go

@@ -114,14 +114,20 @@ func StartApp(
 	provisioner *Provision,
 	modelsIndex *modelsindex.ModelsIndex,
 	bricksIndex *bricksindex.BricksIndex,
-	app app.ArduinoApp,
+	appToStart app.ArduinoApp,
 	cfg config.Configuration,
 	staticStore *store.StaticStore,
 ) iter.Seq[StreamMessage] {
 	return func(yield func(StreamMessage) bool) {
 		ctx, cancel := context.WithCancel(ctx)
 		defer cancel()
 
+		err := app.ValidateBricks(appToStart.Descriptor, bricksIndex)
+		if err != nil {
+			yield(StreamMessage{error: err})
+			return
+		}
+
 		running, err := getRunningApp(ctx, docker.Client())
 		if err != nil {
 			yield(StreamMessage{error: err})
@@ -131,7 +137,7 @@ func StartApp(
 			yield(StreamMessage{error: fmt.Errorf("app %q is running", running.Name)})
 			return
 		}
-		if !yield(StreamMessage{data: fmt.Sprintf("Starting app %q", app.Name)}) {
+		if !yield(StreamMessage{data: fmt.Sprintf("Starting app %q", appToStart.Name)}) {
 			return
 		}
 
@@ -148,11 +154,11 @@ func StartApp(
 		if !yield(StreamMessage{progress: &Progress{Name: "preparing", Progress: 0.0}}) {
 			return
 		}
-		if app.MainSketchPath != nil {
+		if appToStart.MainSketchPath != nil {
 			if !yield(StreamMessage{progress: &Progress{Name: "sketch compiling and uploading", Progress: 0.0}}) {
 				return
 			}
-			if err := compileUploadSketch(ctx, &app, sketchCallbackWriter); err != nil {
+			if err := compileUploadSketch(ctx, &appToStart, sketchCallbackWriter); err != nil {
 				yield(StreamMessage{error: err})
 				return
 			}
@@ -161,23 +167,23 @@ func StartApp(
 			}
 		}
 
-		if app.MainPythonFile != nil {
-			envs := getAppEnvironmentVariables(app, bricksIndex, modelsIndex)
+		if appToStart.MainPythonFile != nil {
+			envs := getAppEnvironmentVariables(appToStart, bricksIndex, modelsIndex)
 
 			if !yield(StreamMessage{data: "python provisioning"}) {
 				cancel()
 				return
 			}
 			provisionStartProgress := float32(0.0)
-			if app.MainSketchPath != nil {
+			if appToStart.MainSketchPath != nil {
 				provisionStartProgress = 10.0
 			}
 
 			if !yield(StreamMessage{progress: &Progress{Name: "python provisioning", Progress: provisionStartProgress}}) {
 				return
 			}
 
-			if err := provisioner.App(ctx, bricksIndex, &app, cfg, envs, staticStore); err != nil {
+			if err := provisioner.App(ctx, bricksIndex, &appToStart, cfg, envs, staticStore); err != nil {
 				yield(StreamMessage{error: err})
 				return
 			}
@@ -188,10 +194,10 @@ func StartApp(
 			}
 
 			// Launch the docker compose command to start the app
-			overrideComposeFile := app.AppComposeOverrideFilePath()
+			overrideComposeFile := appToStart.AppComposeOverrideFilePath()
 
 			commands := []string{}
-			commands = append(commands, "docker", "compose", "-f", app.AppComposeFilePath().String())
+			commands = append(commands, "docker", "compose", "-f", appToStart.AppComposeFilePath().String())
 			if ok, _ := overrideComposeFile.ExistCheck(); ok {
 				commands = append(commands, "-f", overrideComposeFile.String())
 			}
@@ -446,6 +452,13 @@ func RestartApp(
 	return func(yield func(StreamMessage) bool) {
 		ctx, cancel := context.WithCancel(ctx)
 		defer cancel()
+
+		err := app.ValidateBricks(appToStart.Descriptor, bricksIndex)
+		if err != nil {
+			yield(StreamMessage{error: err})
+			return
+		}
+
 		runningApp, err := getRunningApp(ctx, docker.Client())
 		if err != nil {
 			yield(StreamMessage{error: err})