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/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/missing-required-app.yaml b/internal/orchestrator/app/testdata/validator/missing-required-app.yaml new file mode 100644 index 00000000..36a8f903 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/internal/orchestrator/app/testdata/validator/mixed-required-app.yaml b/internal/orchestrator/app/testdata/validator/mixed-required-app.yaml new file mode 100644 index 00000000..7899bd0d --- /dev/null +++ b/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" \ 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..6a688d81 --- /dev/null +++ b/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 \ 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..3f447430 --- /dev/null +++ b/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 +} diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go new file mode 100644 index 00000000..c49bd147 --- /dev/null +++ b/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()) + }) + +} 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