diff --git a/internal/api/handlers/bricks.go b/internal/api/handlers/bricks.go index 7e95753a..9ac76632 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 } @@ -213,7 +213,7 @@ func HandleBrickUpdates( req.ID = id err = brickService.BrickUpdate(req, app) if err != nil { - slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error())) + slog.Error("Unable to update the brick", slog.String("error", err.Error())) render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to update the brick"}) return diff --git a/internal/orchestrator/app/validator.go b/internal/orchestrator/app/validator.go new file mode 100644 index 00000000..fde50548 --- /dev/null +++ b/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 validation errors as a single joined error, allowing the caller to see all issues at once rather than stopping at the first error. +func ValidateBricks(a AppDescriptor, index *bricksindex.BricksIndex) error { + if index == nil { + return fmt.Errorf("bricks index cannot be 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 appBrickVariableName := range appBrick.Variables { + _, exist := indexBrick.GetVariable(appBrickVariableName) + if !exist { + // TODO: we should return warnings + slog.Warn("[skip] variable does not exist into the brick definition", "variable", appBrickVariableName, "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 +} diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go new file mode 100644 index 00000000..13e3e9d3 --- /dev/null +++ b/internal/orchestrator/app/validator_test.go @@ -0,0 +1,163 @@ +package app + +import ( + "errors" + "os" + "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 := &bricksindex.BricksIndex{ + Bricks: []bricksindex.Brick{ + { + ID: "arduino:arduino_cloud", + Name: "Arduino Cloud", + Description: "Connects to Arduino Cloud", + Variables: []bricksindex.BrickVariable{ + { + Name: "ARDUINO_DEVICE_ID", + Description: "Arduino Cloud Device ID", + DefaultValue: "", // Required (no default value) + }, + { + Name: "ARDUINO_SECRET", + Description: "Arduino Cloud Secret", + DefaultValue: "", // Required (no default value) + }, + }, + }, + }, + } + + testCases := []struct { + name string + yamlContent string + expectedError error + }{ + { + name: "valid with all required filled", + yamlContent: ` +name: App ok +description: App ok +bricks: + - arduino:arduino_cloud: + variables: + ARDUINO_DEVICE_ID: "my-device-id" + ARDUINO_SECRET: "my-secret" +`, + expectedError: nil, + }, + { + name: "valid with missing bricks", + yamlContent: ` +name: App with no bricks +description: App with no bricks description +`, + expectedError: nil, + }, + { + name: "valid with empty list of bricks", + yamlContent: ` +name: App with empty bricks +description: App with empty bricks + +bricks: [] +`, + expectedError: nil, + }, + { + name: "valid if required variable is empty string", + yamlContent: ` +name: App with an empty variable +description: App with an empty variable +bricks: + - arduino:arduino_cloud: + variables: + ARDUINO_DEVICE_ID: "my-device-id" + ARDUINO_SECRET: +`, + expectedError: nil, + }, + { + name: "invalid if required variable is omitted", + yamlContent: ` +name: App with no required variables +description: App with no required variables +bricks: + - arduino:arduino_cloud +`, + 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", + yamlContent: ` +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" +`, + expectedError: errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""), + }, + { + name: "invalid if brick id not found", + yamlContent: ` +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" +`, + expectedError: errors.New("brick \"arduino:not_existing_brick\" not found"), + }, + { + name: "log a warning if variable does not exist in the brick", + yamlContent: ` +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" +`, + expectedError: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tempDir := t.TempDir() + err := paths.New(tempDir).MkdirAll() + require.NoError(t, err) + appYaml := paths.New(tempDir, "app.yaml") + err = os.WriteFile(appYaml.String(), []byte(tc.yamlContent), 0600) + require.NoError(t, err) + + appDescriptor, err := ParseDescriptorFile(appYaml) + 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..691249d0 100644 --- a/internal/orchestrator/bricks/bricks.go +++ b/internal/orchestrator/bricks/bricks.go @@ -282,15 +282,15 @@ 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("required variable %q cannot be empty", name) } } for _, brickVar := range brick.Variables { - if brickVar.DefaultValue == "" { + if brickVar.IsRequired() { 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, "brick", brickVar.Name) } } } @@ -335,16 +335,21 @@ func (s *Service) BrickUpdate( req BrickCreateUpdateRequest, appCurrent app.ArduinoApp, ) error { - 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) + brickFromIndex, present := s.bricksIndex.FindBrickByID(req.ID) + if !present { + return fmt.Errorf("brick %q not found into the brick index", req.ID) + } + + brickPosition := slices.IndexFunc(appCurrent.Descriptor.Bricks, func(b app.Brick) bool { return b.ID == req.ID }) + if brickPosition == -1 { + 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 + + brickVariables := appCurrent.Descriptor.Bricks[brickPosition].Variables if len(brickVariables) == 0 { brickVariables = make(map[string]string) } - brickModel := appCurrent.Descriptor.Bricks[index].Model + brickModel := appCurrent.Descriptor.Bricks[brickPosition].Model if req.Model != nil && *req.Model != brickModel { models := s.modelsIndex.GetModelsByBrick(req.ID) @@ -354,17 +359,14 @@ func (s *Service) BrickUpdate( } brickModel = *req.Model } - brick, present := s.bricksIndex.FindBrickByID(brickID) - if !present { - return fmt.Errorf("brick not found with id %s", brickID) - } + for name, updateValue := range req.Variables { - value, exist := brick.GetVariable(name) + value, exist := brickFromIndex.GetVariable(name) if !exist { - return errors.New("variable does not exist") + return fmt.Errorf("variable %q does not exist on brick %q", name, brickFromIndex.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,14 +376,13 @@ func (s *Service) BrickUpdate( break } } - if !updated { brickVariables[name] = updateValue } } - appCurrent.Descriptor.Bricks[index].Model = brickModel - appCurrent.Descriptor.Bricks[index].Variables = brickVariables + appCurrent.Descriptor.Bricks[brickPosition].Model = brickModel + appCurrent.Descriptor.Bricks[brickPosition].Variables = brickVariables err := appCurrent.Save() if err != nil { diff --git a/internal/orchestrator/bricks/bricks_test.go b/internal/orchestrator/bricks/bricks_test.go index 2f03d96b..c14cebfd 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/internal/orchestrator/bricks/bricks_test.go @@ -53,16 +53,27 @@ func TestBrickCreate(t *testing.T) { }} err = brickService.BrickCreate(req, f.Must(app.Load("testdata/dummy-app"))) require.Error(t, err) - require.Equal(t, "variable \"ARDUINO_DEVICE_ID\" cannot be empty", err.Error()) + require.Equal(t, "required 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) { @@ -79,6 +90,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() @@ -111,6 +123,122 @@ func TestBrickCreate(t *testing.T) { }) } +func TestUpdateBrick(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 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 \"arduino:dbstorage_sqlstore\" not found into the bricks of the app", 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()) + }) + + // TODO: allow to set an empty "" 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.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("allow updating only one mandatory variable among two", 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.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("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")) + 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"]) + }) + + 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) { tests := []struct { name string diff --git a/internal/orchestrator/bricks/testdata/dummy-app-for-update/app.yaml b/internal/orchestrator/bricks/testdata/dummy-app-for-update/app.yaml new file mode 100644 index 00000000..5b168ed5 --- /dev/null +++ b/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" \ No newline at end of file diff --git a/internal/orchestrator/bricks/testdata/dummy-app-for-update/python/main.py b/internal/orchestrator/bricks/testdata/dummy-app-for-update/python/main.py new file mode 100644 index 00000000..336e825c --- /dev/null +++ b/internal/orchestrator/bricks/testdata/dummy-app-for-update/python/main.py @@ -0,0 +1,2 @@ +def main(): + pass diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 19181998..23f71d57 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()) }