diff --git a/internal/api/handlers/bricks.go b/internal/api/handlers/bricks.go index 7e95753a..f392aabc 100644 --- a/internal/api/handlers/bricks.go +++ b/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 } diff --git a/internal/orchestrator/app/testdata/validator/all-required-app.yaml b/internal/orchestrator/app/testdata/validator/all-required-app.yaml new file mode 100644 index 00000000..4f38ee02 --- /dev/null +++ b/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" \ No newline at end of file diff --git a/internal/orchestrator/app/testdata/validator/bricks-list.yaml b/internal/orchestrator/app/testdata/validator/bricks-list.yaml new file mode 100644 index 00000000..113d0077 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/internal/orchestrator/app/testdata/validator/empty-bricks-app.yaml b/internal/orchestrator/app/testdata/validator/empty-bricks-app.yaml new file mode 100644 index 00000000..8733c5d4 --- /dev/null +++ b/internal/orchestrator/app/testdata/validator/empty-bricks-app.yaml @@ -0,0 +1,4 @@ +name: App with empty bricks +description: App with empty bricks + +bricks: [] diff --git a/internal/orchestrator/app/testdata/validator/empty-required-app.yaml b/internal/orchestrator/app/testdata/validator/empty-required-app.yaml new file mode 100644 index 00000000..3b357718 --- /dev/null +++ b/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: \ No newline at end of file diff --git a/internal/orchestrator/app/testdata/validator/no-bricks-app.yaml b/internal/orchestrator/app/testdata/validator/no-bricks-app.yaml new file mode 100644 index 00000000..915a18ea --- /dev/null +++ b/internal/orchestrator/app/testdata/validator/no-bricks-app.yaml @@ -0,0 +1,2 @@ +name: App with no bricks +description: App with no bricks description \ No newline at end of file diff --git a/internal/orchestrator/app/testdata/validator/not-found-brick-app.yaml b/internal/orchestrator/app/testdata/validator/not-found-brick-app.yaml new file mode 100644 index 00000000..59f76616 --- /dev/null +++ b/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" \ No newline at end of file diff --git a/internal/orchestrator/app/testdata/validator/not-found-variable-app.yaml b/internal/orchestrator/app/testdata/validator/not-found-variable-app.yaml new file mode 100644 index 00000000..44adafd2 --- /dev/null +++ b/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" \ No newline at end of file diff --git a/internal/orchestrator/app/testdata/validator/omitted-mixed-required-app.yaml b/internal/orchestrator/app/testdata/validator/omitted-mixed-required-app.yaml new file mode 100644 index 00000000..7899bd0d --- /dev/null +++ b/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" \ No newline at end of file diff --git a/internal/orchestrator/app/testdata/validator/omitted-required-app.yaml b/internal/orchestrator/app/testdata/validator/omitted-required-app.yaml new file mode 100644 index 00000000..36a8f903 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/internal/orchestrator/app/validator.go b/internal/orchestrator/app/validator.go new file mode 100644 index 00000000..aa184a19 --- /dev/null +++ b/internal/orchestrator/app/validator.go @@ -0,0 +1,44 @@ +package app + +import ( + "errors" + "fmt" + + "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 { + allErrors = errors.Join(allErrors, fmt.Errorf("variable %q does not exist on brick %q", appBrickName, 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 +} diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go new file mode 100644 index 00000000..2c10499b --- /dev/null +++ b/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: "invalid if variable does not exist in the brick", + filename: "not-found-variable-app.yaml", + expectedError: errors.New("variable \"NOT_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\""), + }, + } + + 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") + } + }) + } +} diff --git a/internal/orchestrator/bricks/bricks.go b/internal/orchestrator/bricks/bricks.go index e8dea9da..dc59d941 100644 --- a/internal/orchestrator/bricks/bricks.go +++ b/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) } } } diff --git a/internal/orchestrator/bricks/bricks_test.go b/internal/orchestrator/bricks/bricks_test.go index 2f03d96b..bfb5cb75 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/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) { diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 19181998..e86c942c 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -114,7 +114,7 @@ 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] { @@ -122,6 +122,12 @@ func StartApp( 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,15 +167,15 @@ 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 } @@ -177,7 +183,7 @@ func StartApp( 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})