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/parser.go b/internal/orchestrator/app/parser.go index 4bff1ba1..c0d5e3c4 100644 --- a/internal/orchestrator/app/parser.go +++ b/internal/orchestrator/app/parser.go @@ -24,6 +24,8 @@ import ( "github.com/arduino/go-paths-helper" "github.com/goccy/go-yaml" "github.com/goccy/go-yaml/ast" + + "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" ) type Brick struct { @@ -138,6 +140,44 @@ func (a *AppDescriptor) IsValid() error { return allErrors } +// ValidateBricks checks that all bricks referenced in the given AppDescriptor exist in the provided BricksIndex, +// and that all required variables for each brick are present and valid. It collects and returns all validation +// errors found as a slice, allowing the caller to see all issues at once rather than stopping at the first error. +// If the index is nil, validation is skipped and nil is returned. +func (a *AppDescriptor) ValidateBricks(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 = append(allErrors, fmt.Errorf("brick %q not found", appBrick.ID)) + continue // Skip further validation for this brick since it doesn't exist + } + + // Check that all app variables exist in brick definition + for appBrickName := range appBrick.Variables { + _, exist := indexBrick.GetVariable(appBrickName) + if !exist { + allErrors = append(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 = append(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID)) + } + } + } + } + return allErrors +} + func isSingleEmoji(s string) bool { emojis := 0 for it := emoji.IterateChars(s); it.Next(); { 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_test.go b/internal/orchestrator/app/validator_test.go new file mode 100644 index 00000000..19b4ed03 --- /dev/null +++ b/internal/orchestrator/app/validator_test.go @@ -0,0 +1,91 @@ +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 + expectedErrors []error // Now expecting a slice of error messages + }{ + { + name: "valid with all required filled", + filename: "all-required-app.yaml", + expectedErrors: nil, + }, + { + name: "valid with missing bricks", + filename: "no-bricks-app.yaml", + expectedErrors: nil, + }, + { + name: "valid with empty list of bricks", + filename: "empty-bricks-app.yaml", + expectedErrors: nil, + }, + { + name: "valid if required variable is empty string", + filename: "empty-required-app.yaml", + expectedErrors: nil, + }, + { + name: "invalid if required variable is omitted", + filename: "omitted-required-app.yaml", + expectedErrors: []error{ + 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", + expectedErrors: []error{ + 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", + expectedErrors: []error{ + 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", + expectedErrors: []error{ + 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) + + errs := appDescriptor.ValidateBricks(bricksIndex) + if tc.expectedErrors == nil { + require.Nil(t, errs) + assert.Empty(t, errs, "Expected no validation errors") + } else { + require.Len(t, errs, len(tc.expectedErrors), "Expected %d errors but got %d", len(tc.expectedErrors), len(errs)) + require.Exactly(t, tc.expectedErrors, errs) + } + }) + } +} diff --git a/internal/orchestrator/bricks/bricks.go b/internal/orchestrator/bricks/bricks.go index e8dea9da..50459be1 100644 --- a/internal/orchestrator/bricks/bricks.go +++ b/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) } } } diff --git a/internal/orchestrator/bricks/bricks_test.go b/internal/orchestrator/bricks/bricks_test.go index 2f03d96b..f9974a42 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/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 diff --git a/internal/orchestrator/bricks/testdata/dummy-app/app.yaml b/internal/orchestrator/bricks/testdata/AppFromExample/app.yaml similarity index 100% rename from internal/orchestrator/bricks/testdata/dummy-app/app.yaml rename to internal/orchestrator/bricks/testdata/AppFromExample/app.yaml diff --git a/internal/orchestrator/bricks/testdata/dummy-app/python/main.py b/internal/orchestrator/bricks/testdata/AppFromExample/python/main.py similarity index 100% rename from internal/orchestrator/bricks/testdata/dummy-app/python/main.py rename to internal/orchestrator/bricks/testdata/AppFromExample/python/main.py diff --git a/internal/orchestrator/bricks/testdata/app.golden.yaml b/internal/orchestrator/bricks/testdata/app.golden.yaml new file mode 100644 index 00000000..e69de29b diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 19181998..37413640 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -122,6 +122,16 @@ func StartApp( ctx, cancel := context.WithCancel(ctx) defer cancel() + errs := app.Descriptor.ValidateBricks(bricksIndex) + if errs != nil { + for _, e := range errs { + if !yield(StreamMessage{error: e}) { + return + } + } + return + } + running, err := getRunningApp(ctx, docker.Client()) if err != nil { yield(StreamMessage{error: err}) @@ -446,6 +456,17 @@ func RestartApp( return func(yield func(StreamMessage) bool) { ctx, cancel := context.WithCancel(ctx) defer cancel() + + errs := appToStart.Descriptor.ValidateBricks(bricksIndex) + if errs != nil { + for _, e := range errs { + if !yield(StreamMessage{error: e}) { + return + } + } + return + } + runningApp, err := getRunningApp(ctx, docker.Client()) if err != nil { yield(StreamMessage{error: err})