From 4df8f588811c7960a1810b294b91adc1def1064b Mon Sep 17 00:00:00 2001 From: dido18 Date: Wed, 12 Nov 2025 12:16:02 +0100 Subject: [PATCH 1/8] fix: improve error logging message in HandleBrickCreate function --- internal/api/handlers/bricks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 } From 650906d49e59f06701a608f58ad65cbb0bae321e Mon Sep 17 00:00:00 2001 From: dido18 Date: Wed, 12 Nov 2025 16:03:06 +0100 Subject: [PATCH 2/8] test: rename TestBrickCreate to TestBrickCreateFromAppExample and update test data feat: add AppFromExample configuration and Python main file for testing --- internal/orchestrator/bricks/bricks_test.go | 40 ++++++++++--------- .../{dummy-app => AppFromExample}/app.yaml | 8 ++-- .../python/main.py | 0 3 files changed, 26 insertions(+), 22 deletions(-) rename internal/orchestrator/bricks/testdata/{dummy-app => AppFromExample}/app.yaml (53%) rename internal/orchestrator/bricks/testdata/{dummy-app => AppFromExample}/python/main.py (100%) diff --git a/internal/orchestrator/bricks/bricks_test.go b/internal/orchestrator/bricks/bricks_test.go index 2f03d96b..06e301ee 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/internal/orchestrator/bricks/bricks_test.go @@ -26,13 +26,13 @@ import ( "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" ) -func TestBrickCreate(t *testing.T) { +func TestBrickCreateFromAppExample(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", 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,7 +41,7 @@ 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()) }) @@ -51,7 +51,7 @@ func TestBrickCreate(t *testing.T) { "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()) }) @@ -60,31 +60,27 @@ func TestBrickCreate(t *testing.T) { 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"))) + err = brickService.BrickCreate(req, f.Must(app.Load("testdata/AppFromExample"))) require.Error(t, err) require.Equal(t, "required variable \"ARDUINO_DEVICE_ID\" is mandatory", err.Error()) }) 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 +95,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 +107,14 @@ func TestBrickCreate(t *testing.T) { }) } +func copyToTempApp(t *testing.T, srcApp *paths.Path) (tmpApp *paths.Path, cleanUp func()) { + tmpAppPath := paths.New(srcApp.String() + ".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 53% rename from internal/orchestrator/bricks/testdata/dummy-app/app.yaml rename to internal/orchestrator/bricks/testdata/AppFromExample/app.yaml index 281821c6..8af8e098 100644 --- a/internal/orchestrator/bricks/testdata/dummy-app/app.yaml +++ b/internal/orchestrator/bricks/testdata/AppFromExample/app.yaml @@ -1,6 +1,6 @@ -name: Copy of Blinking LED from Arduino Cloud -description: Control the LED from the Arduino IoT Cloud using RPC calls +name: Blinking LED from Arduino Cloud icon: ☁️ -ports: [] +description: Control the LED from the Arduino IoT Cloud using RPC calls + bricks: -- arduino:arduino_cloud: \ No newline at end of file + - arduino:arduino_cloud \ No newline at end of file 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 From 59a73d2f9282388a5cec1d450b6d5b8a7db6ba58 Mon Sep 17 00:00:00 2001 From: dido18 Date: Wed, 12 Nov 2025 17:30:24 +0100 Subject: [PATCH 3/8] fix: log a warning for missing mandatory variables in BrickCreate and update test cases --- internal/orchestrator/bricks/bricks.go | 4 +++- internal/orchestrator/bricks/bricks_test.go | 21 ++++++++++++++++----- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/internal/orchestrator/bricks/bricks.go b/internal/orchestrator/bricks/bricks.go index e8dea9da..183a46c8 100644 --- a/internal/orchestrator/bricks/bricks.go +++ b/internal/orchestrator/bricks/bricks.go @@ -290,7 +290,9 @@ 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) + // PATCH: to allow the AppLab to add a brick to a app created from scratch becasue currently + // the FE does not send the required variables in the request. + slog.Warn("[Skip] variable has no default value and it 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 06e301ee..525c1d02 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/internal/orchestrator/bricks/bricks_test.go @@ -17,6 +17,7 @@ package bricks import ( "testing" + "time" "github.com/arduino/go-paths-helper" "github.com/stretchr/testify/require" @@ -46,6 +47,7 @@ func TestBrickCreateFromAppExample(t *testing.T) { require.Equal(t, "variable \"NON_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\"", err.Error()) }) + //TODO: allow a variable to have empty string 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": "", @@ -56,13 +58,22 @@ func TestBrickCreateFromAppExample(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("log a warning if 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/AppFromExample"))) - 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) + require.Equal(t, "", after.Descriptor.Bricks[0].Variables["ARDUINO_DEVICE_ID"]) // <-- the DEVICE_ID is set to empty + 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) { @@ -108,7 +119,7 @@ func TestBrickCreateFromAppExample(t *testing.T) { } func copyToTempApp(t *testing.T, srcApp *paths.Path) (tmpApp *paths.Path, cleanUp func()) { - tmpAppPath := paths.New(srcApp.String() + ".temp") + tmpAppPath := paths.New(srcApp.String() + time.Now().Format(time.RFC3339) + ".temp") require.Nil(t, srcApp.CopyDirTo(tmpAppPath)) return tmpAppPath, func() { require.Nil(t, tmpAppPath.RemoveAll()) From 18a0030f4553c4a070dff755bc0e650bb1b1a169 Mon Sep 17 00:00:00 2001 From: dido18 Date: Wed, 12 Nov 2025 17:43:58 +0100 Subject: [PATCH 4/8] revert some changes --- internal/orchestrator/bricks/bricks_test.go | 2 +- .../orchestrator/bricks/testdata/AppFromExample/app.yaml | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/orchestrator/bricks/bricks_test.go b/internal/orchestrator/bricks/bricks_test.go index 525c1d02..edcad8d3 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/internal/orchestrator/bricks/bricks_test.go @@ -27,7 +27,7 @@ import ( "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" ) -func TestBrickCreateFromAppExample(t *testing.T) { +func TestBrickCreate(t *testing.T) { bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata")) require.Nil(t, err) brickService := NewService(nil, bricksIndex, nil) diff --git a/internal/orchestrator/bricks/testdata/AppFromExample/app.yaml b/internal/orchestrator/bricks/testdata/AppFromExample/app.yaml index 8af8e098..281821c6 100644 --- a/internal/orchestrator/bricks/testdata/AppFromExample/app.yaml +++ b/internal/orchestrator/bricks/testdata/AppFromExample/app.yaml @@ -1,6 +1,6 @@ -name: Blinking LED from Arduino Cloud -icon: ☁️ +name: Copy of Blinking LED from Arduino Cloud description: Control the LED from the Arduino IoT Cloud using RPC calls - +icon: ☁️ +ports: [] bricks: - - arduino:arduino_cloud \ No newline at end of file +- arduino:arduino_cloud: \ No newline at end of file From a8c8314a4aa82c734d350b008a09a025baba9619 Mon Sep 17 00:00:00 2001 From: dido18 Date: Wed, 12 Nov 2025 17:48:43 +0100 Subject: [PATCH 5/8] fix: update test cases for BrickCreate to improve clarity and logging --- internal/orchestrator/bricks/bricks_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/orchestrator/bricks/bricks_test.go b/internal/orchestrator/bricks/bricks_test.go index edcad8d3..1d98a468 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/internal/orchestrator/bricks/bricks_test.go @@ -16,6 +16,7 @@ package bricks import ( + "fmt" "testing" "time" @@ -47,7 +48,7 @@ func TestBrickCreate(t *testing.T) { require.Equal(t, "variable \"NON_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\"", err.Error()) }) - //TODO: allow a variable to have empty string + //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": "", @@ -59,8 +60,8 @@ func TestBrickCreate(t *testing.T) { }) t.Run("log a warning if a mandatory variable is not present in the request", func(t *testing.T) { - tempApp, cleanUp := copyToTempApp(t, paths.New("testdata/AppFromExample")) - defer cleanUp() + tempApp, _ := copyToTempApp(t, paths.New("testdata/AppFromExample")) + // defer cleanUp() req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{ "ARDUINO_SECRET": "a-secret-a", @@ -72,7 +73,7 @@ func TestBrickCreate(t *testing.T) { 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"]) // <-- the DEVICE_ID is set to empty + require.Equal(t, "", after.Descriptor.Bricks[0].Variables["ARDUINO_DEVICE_ID"]) // <-- the DEVICE_ID is empty require.Equal(t, "a-secret-a", after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"]) }) @@ -119,7 +120,7 @@ func TestBrickCreate(t *testing.T) { } func copyToTempApp(t *testing.T, srcApp *paths.Path) (tmpApp *paths.Path, cleanUp func()) { - tmpAppPath := paths.New(srcApp.String() + time.Now().Format(time.RFC3339) + ".temp") + 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()) From 289e6afc9a0897800028aa65b189c6a5a1c0bc61 Mon Sep 17 00:00:00 2001 From: dido18 Date: Wed, 12 Nov 2025 18:03:34 +0100 Subject: [PATCH 6/8] fix: correct typo in comment for BrickCreate function to enhance clarity --- internal/orchestrator/bricks/bricks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/orchestrator/bricks/bricks.go b/internal/orchestrator/bricks/bricks.go index 183a46c8..9614fe58 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 { - // PATCH: to allow the AppLab to add a brick to a app created from scratch becasue currently + // PATCH: to allow the AppLab to add a brick to a app created from scratch because currently // the FE does not send the required variables in the request. slog.Warn("[Skip] variable has no default value and it is not set by user", "variable", brickVar.Name) } From ad54d74f29408831d2db4dccfe095bf65416baaf Mon Sep 17 00:00:00 2001 From: dido18 Date: Wed, 12 Nov 2025 18:57:16 +0100 Subject: [PATCH 7/8] fix: improve warning message for missing mandatory variables in BrickCreate and update test case descriptions --- internal/orchestrator/bricks/bricks.go | 4 +--- internal/orchestrator/bricks/bricks_test.go | 11 +++++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/internal/orchestrator/bricks/bricks.go b/internal/orchestrator/bricks/bricks.go index 9614fe58..50459be1 100644 --- a/internal/orchestrator/bricks/bricks.go +++ b/internal/orchestrator/bricks/bricks.go @@ -290,9 +290,7 @@ func (s *Service) BrickCreate( for _, brickVar := range brick.Variables { if brickVar.DefaultValue == "" { if _, exist := req.Variables[brickVar.Name]; !exist { - // PATCH: to allow the AppLab to add a brick to a app created from scratch because currently - // the FE does not send the required variables in the request. - slog.Warn("[Skip] variable has no default value and it is not set by user", "variable", 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 1d98a468..f9974a42 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/internal/orchestrator/bricks/bricks_test.go @@ -59,9 +59,9 @@ func TestBrickCreate(t *testing.T) { require.Equal(t, "variable \"ARDUINO_DEVICE_ID\" cannot be empty", err.Error()) }) - t.Run("log a warning if a mandatory variable is not present in the request", func(t *testing.T) { - tempApp, _ := copyToTempApp(t, paths.New("testdata/AppFromExample")) - // defer cleanUp() + 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", @@ -73,7 +73,10 @@ func TestBrickCreate(t *testing.T) { 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"]) // <-- the DEVICE_ID is empty + // 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"]) }) From 1e8e541661bfe0ffaf350fb52e119e7ae75847a2 Mon Sep 17 00:00:00 2001 From: dido18 Date: Thu, 13 Nov 2025 22:26:06 +0100 Subject: [PATCH 8/8] feat: add validation for app descriptors and corresponding test cases --- .../app/testdata/validator/bricks-list.yaml | 9 ++++ .../testdata/validator/empty-bricks-app.yaml | 4 ++ .../validator/missing-required-app.yaml | 4 ++ .../validator/mixed-required-app.yaml | 6 +++ .../app/testdata/validator/no-bricks-app.yaml | 2 + .../validator/not-found-brick-app.yaml | 4 ++ internal/orchestrator/app/validator.go | 38 +++++++++++++ internal/orchestrator/app/validator_test.go | 53 +++++++++++++++++++ .../bricks/testdata/app.golden.yaml | 0 9 files changed, 120 insertions(+) create mode 100644 internal/orchestrator/app/testdata/validator/bricks-list.yaml create mode 100644 internal/orchestrator/app/testdata/validator/empty-bricks-app.yaml create mode 100644 internal/orchestrator/app/testdata/validator/missing-required-app.yaml create mode 100644 internal/orchestrator/app/testdata/validator/mixed-required-app.yaml create mode 100644 internal/orchestrator/app/testdata/validator/no-bricks-app.yaml create mode 100644 internal/orchestrator/app/testdata/validator/not-found-brick-app.yaml create mode 100644 internal/orchestrator/app/validator.go create mode 100644 internal/orchestrator/app/validator_test.go create mode 100644 internal/orchestrator/bricks/testdata/app.golden.yaml 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/testdata/app.golden.yaml b/internal/orchestrator/bricks/testdata/app.golden.yaml new file mode 100644 index 00000000..e69de29b