From 4df8f588811c7960a1810b294b91adc1def1064b Mon Sep 17 00:00:00 2001 From: dido18 Date: Wed, 12 Nov 2025 12:16:02 +0100 Subject: [PATCH 01/30] 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 02/30] 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 03/30] 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 04/30] 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 05/30] 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 06/30] 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 07/30] 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 08/30] 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 From 58a42271c33514f47a5dfb4dcc3e62bfd76fb683 Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 14:09:55 +0100 Subject: [PATCH 09/30] fix: correct expected error message for missing required variable in app descriptor test --- internal/orchestrator/app/validator_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go index c49bd147..59a0456c 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -40,7 +40,7 @@ func TestValidateAppDescriptor(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()) + assert.Equal(t, "variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\"", err.Error()) }) t.Run("invalid app descriptor with not found brick id", func(t *testing.T) { From 7878a2c8e0fbe0ee6f9b0efa522453e6aeb23d98 Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 14:18:20 +0100 Subject: [PATCH 10/30] refactor: rename validation functions and update test cases for clarity and consistency --- internal/orchestrator/app/validator.go | 15 ++-- internal/orchestrator/app/validator_test.go | 83 ++++++++++++--------- 2 files changed, 56 insertions(+), 42 deletions(-) diff --git a/internal/orchestrator/app/validator.go b/internal/orchestrator/app/validator.go index 3f447430..50ba6f28 100644 --- a/internal/orchestrator/app/validator.go +++ b/internal/orchestrator/app/validator.go @@ -6,18 +6,20 @@ import ( "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 { +// 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 returns an error if any brick is missing, +// if any variable referenced by the app does not exist in the corresponding brick, or if any required variable is missing. +// If the index is nil, validation is skipped and nil is returned. +func ValidateBricks(a AppDescriptor, index *bricksindex.BricksIndex) error { + if index == nil { + return nil + } 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 { @@ -25,7 +27,6 @@ func validatebricks(a AppDescriptor, index *bricksindex.BricksIndex) error { } } - // check all required variables has a value for _, indexBrickVariable := range indexBrick.Variables { if indexBrickVariable.IsRequired() { if _, exist := appBrick.Variables[indexBrickVariable.Name]; !exist { diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go index 59a0456c..98d5405e 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -5,49 +5,62 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.bug.st/f" "github.com/arduino/go-paths-helper" "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" ) -func TestValidateAppDescriptor(t *testing.T) { +func TestValidateBricksOnAppDescriptor(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_SECRET\" 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()) - }) + testCases := []struct { + name string + filename string + expectedErr *string + }{ + { + name: "valid app descriptor with no bricks", + filename: "no-bricks-app.yaml", + expectedErr: nil, + }, + { + name: "valid app descriptor with empty list of bricks", + filename: "empty-bricks-app.yaml", + expectedErr: nil, + }, + { + name: "invalid app descriptor with missing required variable", + filename: "missing-required-app.yaml", + expectedErr: f.Ptr("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\""), + }, + { + name: "invalid app descriptor with a missing required variable among two", + filename: "mixed-required-app.yaml", + expectedErr: f.Ptr("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""), + }, + { + name: "invalid app descriptor with not found brick id", + filename: "not-found-brick-app.yaml", + expectedErr: f.Ptr("brick \"arduino:not_existing_brick\" not found"), + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + app, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename)) + require.NoError(t, err) + + err = ValidateBricks(app, bricksIndex) + + if tc.expectedErr == nil { + assert.NoError(t, err) + } else { + require.Error(t, err) + assert.Equal(t, *tc.expectedErr, err.Error()) + } + }) + } } From ef6c2deb8a7b81e9f26c3214741a70d392edd283 Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 14:20:18 +0100 Subject: [PATCH 11/30] fix: rename variable for clarity and validate app descriptor in HandleAppStart --- internal/api/handlers/app_start.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/api/handlers/app_start.go b/internal/api/handlers/app_start.go index 0293aff0..fde915e8 100644 --- a/internal/api/handlers/app_start.go +++ b/internal/api/handlers/app_start.go @@ -47,7 +47,7 @@ func HandleAppStart( return } - app, err := app.Load(id.ToPath().String()) + appLoaded, err := app.Load(id.ToPath().String()) if err != nil { slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()), slog.String("path", id.String())) render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to find the app"}) @@ -62,6 +62,14 @@ func HandleAppStart( } defer sseStream.Close() + err = app.ValidateBricks(appLoaded.Descriptor, bricksIndex) + if err != nil { + sseStream.SendError(render.SSEErrorData{ + Code: render.InternalServiceErr, + Message: err.Error(), + }) + } + type progress struct { Name string `json:"name"` Progress float32 `json:"progress"` @@ -69,7 +77,7 @@ func HandleAppStart( type log struct { Message string `json:"message"` } - for item := range orchestrator.StartApp(r.Context(), dockerCli, provisioner, modelsIndex, bricksIndex, app, cfg, staticStore) { + for item := range orchestrator.StartApp(r.Context(), dockerCli, provisioner, modelsIndex, bricksIndex, appLoaded, cfg, staticStore) { switch item.GetType() { case orchestrator.ProgressType: sseStream.Send(render.SSEEvent{Type: "progress", Data: progress(*item.GetProgress())}) From 84f1256138f532ce362ad96ea367173298f19950 Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 14:25:01 +0100 Subject: [PATCH 12/30] fix: update error code to BadRequestErr in HandleAppStart and add corresponding constant --- internal/api/handlers/app_start.go | 3 ++- internal/render/sse.go | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/api/handlers/app_start.go b/internal/api/handlers/app_start.go index fde915e8..5360cc1a 100644 --- a/internal/api/handlers/app_start.go +++ b/internal/api/handlers/app_start.go @@ -65,9 +65,10 @@ func HandleAppStart( err = app.ValidateBricks(appLoaded.Descriptor, bricksIndex) if err != nil { sseStream.SendError(render.SSEErrorData{ - Code: render.InternalServiceErr, + Code: render.BadRequestErr, Message: err.Error(), }) + return } type progress struct { diff --git a/internal/render/sse.go b/internal/render/sse.go index 3f269f9a..861e5055 100644 --- a/internal/render/sse.go +++ b/internal/render/sse.go @@ -30,6 +30,7 @@ type SSEErrCode string const ( InternalServiceErr SSEErrCode = "INTERNAL_SERVER_ERROR" + BadRequestErr SSEErrCode = "BAD_REQUEST_ERROR" ) type SSEErrorData struct { From 9bc05aceae3fe85988fec477111de38c6d33123b Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 14:51:24 +0100 Subject: [PATCH 13/30] refactor: consolidate brick validation into AppDescriptor and update related tests --- internal/api/handlers/app_start.go | 13 ++----- internal/orchestrator/app/parser.go | 33 +++++++++++++++++ internal/orchestrator/app/validator.go | 39 --------------------- internal/orchestrator/app/validator_test.go | 7 ++-- internal/orchestrator/orchestrator.go | 13 +++++++ internal/render/sse.go | 1 - 6 files changed, 51 insertions(+), 55 deletions(-) delete mode 100644 internal/orchestrator/app/validator.go diff --git a/internal/api/handlers/app_start.go b/internal/api/handlers/app_start.go index 5360cc1a..0293aff0 100644 --- a/internal/api/handlers/app_start.go +++ b/internal/api/handlers/app_start.go @@ -47,7 +47,7 @@ func HandleAppStart( return } - appLoaded, err := app.Load(id.ToPath().String()) + app, err := app.Load(id.ToPath().String()) if err != nil { slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()), slog.String("path", id.String())) render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to find the app"}) @@ -62,15 +62,6 @@ func HandleAppStart( } defer sseStream.Close() - err = app.ValidateBricks(appLoaded.Descriptor, bricksIndex) - if err != nil { - sseStream.SendError(render.SSEErrorData{ - Code: render.BadRequestErr, - Message: err.Error(), - }) - return - } - type progress struct { Name string `json:"name"` Progress float32 `json:"progress"` @@ -78,7 +69,7 @@ func HandleAppStart( type log struct { Message string `json:"message"` } - for item := range orchestrator.StartApp(r.Context(), dockerCli, provisioner, modelsIndex, bricksIndex, appLoaded, cfg, staticStore) { + for item := range orchestrator.StartApp(r.Context(), dockerCli, provisioner, modelsIndex, bricksIndex, app, cfg, staticStore) { switch item.GetType() { case orchestrator.ProgressType: sseStream.Send(render.SSEEvent{Type: "progress", Data: progress(*item.GetProgress())}) diff --git a/internal/orchestrator/app/parser.go b/internal/orchestrator/app/parser.go index 4bff1ba1..abac997f 100644 --- a/internal/orchestrator/app/parser.go +++ b/internal/orchestrator/app/parser.go @@ -21,6 +21,7 @@ import ( "io" emoji "github.com/Andrew-M-C/go.emoji" + "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" "github.com/arduino/go-paths-helper" "github.com/goccy/go-yaml" "github.com/goccy/go-yaml/ast" @@ -138,6 +139,38 @@ 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 returns an error if any brick is missing, +// if any variable referenced by the app does not exist in the corresponding brick, or if any required variable is missing. +// 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 + } + for _, appBrick := range a.Bricks { + indexBrick, found := index.FindBrickByID(appBrick.ID) + if !found { + return fmt.Errorf("brick %q not found", appBrick.ID) + } + + 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) + } + } + + 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 +} + func isSingleEmoji(s string) bool { emojis := 0 for it := emoji.IterateChars(s); it.Next(); { diff --git a/internal/orchestrator/app/validator.go b/internal/orchestrator/app/validator.go deleted file mode 100644 index 50ba6f28..00000000 --- a/internal/orchestrator/app/validator.go +++ /dev/null @@ -1,39 +0,0 @@ -package app - -import ( - "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, -// and that all required variables for each brick are present and valid. It returns an error if any brick is missing, -// if any variable referenced by the app does not exist in the corresponding brick, or if any required variable is missing. -// If the index is nil, validation is skipped and nil is returned. -func ValidateBricks(a AppDescriptor, index *bricksindex.BricksIndex) error { - if index == nil { - return nil - } - for _, appBrick := range a.Bricks { - indexBrick, found := index.FindBrickByID(appBrick.ID) - if !found { - return fmt.Errorf("brick %q not found", appBrick.ID) - } - - 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) - } - } - - 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 index 98d5405e..5c44a4a6 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -12,7 +12,7 @@ import ( "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" ) -func TestValidateBricksOnAppDescriptor(t *testing.T) { +func TestValidateAppDescriptorBricks(t *testing.T) { bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata/validator")) require.Nil(t, err) @@ -50,11 +50,10 @@ func TestValidateBricksOnAppDescriptor(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - app, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename)) + appDescriptor, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename)) require.NoError(t, err) - err = ValidateBricks(app, bricksIndex) - + appDescriptor.ValidateBricks(bricksIndex) if tc.expectedErr == nil { assert.NoError(t, err) } else { diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 19181998..c6b0499d 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -122,6 +122,12 @@ func StartApp( ctx, cancel := context.WithCancel(ctx) defer cancel() + err := app.Descriptor.ValidateBricks(bricksIndex) + if err != nil { + yield(StreamMessage{error: err}) + return + } + running, err := getRunningApp(ctx, docker.Client()) if err != nil { yield(StreamMessage{error: err}) @@ -446,6 +452,13 @@ func RestartApp( return func(yield func(StreamMessage) bool) { ctx, cancel := context.WithCancel(ctx) defer cancel() + + err := appToStart.Descriptor.ValidateBricks(bricksIndex) + if err != nil { + yield(StreamMessage{error: err}) + return + } + runningApp, err := getRunningApp(ctx, docker.Client()) if err != nil { yield(StreamMessage{error: err}) diff --git a/internal/render/sse.go b/internal/render/sse.go index 861e5055..3f269f9a 100644 --- a/internal/render/sse.go +++ b/internal/render/sse.go @@ -30,7 +30,6 @@ type SSEErrCode string const ( InternalServiceErr SSEErrCode = "INTERNAL_SERVER_ERROR" - BadRequestErr SSEErrCode = "BAD_REQUEST_ERROR" ) type SSEErrorData struct { From 05f12bf74b6541ac68924d43817048dd899be5bd Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 14:54:19 +0100 Subject: [PATCH 14/30] fix: ensure bricks index is not nil and capture error from ValidateBricks in test --- internal/orchestrator/app/validator_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go index 5c44a4a6..13bc36f5 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -15,6 +15,7 @@ import ( 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 @@ -53,7 +54,7 @@ func TestValidateAppDescriptorBricks(t *testing.T) { appDescriptor, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename)) require.NoError(t, err) - appDescriptor.ValidateBricks(bricksIndex) + err = appDescriptor.ValidateBricks(bricksIndex) if tc.expectedErr == nil { assert.NoError(t, err) } else { From 70e1dbb9dbaa7ae525d61bd7132525247454bd7d Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 15:23:37 +0100 Subject: [PATCH 15/30] test: add YAML test cases for app descriptor validation with empty and omitted variables --- .../validator/empty-required-app.yaml | 7 +++++++ ...p.yaml => omitted-mixed-required-app.yaml} | 0 ...red-app.yaml => omitted-required-app.yaml} | 0 internal/orchestrator/app/validator_test.go | 20 ++++++++++++------- 4 files changed, 20 insertions(+), 7 deletions(-) create mode 100644 internal/orchestrator/app/testdata/validator/empty-required-app.yaml rename internal/orchestrator/app/testdata/validator/{mixed-required-app.yaml => omitted-mixed-required-app.yaml} (100%) rename internal/orchestrator/app/testdata/validator/{missing-required-app.yaml => omitted-required-app.yaml} (100%) 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/mixed-required-app.yaml b/internal/orchestrator/app/testdata/validator/omitted-mixed-required-app.yaml similarity index 100% rename from internal/orchestrator/app/testdata/validator/mixed-required-app.yaml rename to internal/orchestrator/app/testdata/validator/omitted-mixed-required-app.yaml diff --git a/internal/orchestrator/app/testdata/validator/missing-required-app.yaml b/internal/orchestrator/app/testdata/validator/omitted-required-app.yaml similarity index 100% rename from internal/orchestrator/app/testdata/validator/missing-required-app.yaml rename to internal/orchestrator/app/testdata/validator/omitted-required-app.yaml diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go index 13bc36f5..b40bfe4c 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -23,27 +23,33 @@ func TestValidateAppDescriptorBricks(t *testing.T) { expectedErr *string }{ { - name: "valid app descriptor with no bricks", + name: "valid with missing bricks", filename: "no-bricks-app.yaml", expectedErr: nil, }, { - name: "valid app descriptor with empty list of bricks", + name: "valid with empty list of bricks", filename: "empty-bricks-app.yaml", expectedErr: nil, }, { - name: "invalid app descriptor with missing required variable", - filename: "missing-required-app.yaml", + name: "valid if required variable is empty string", + filename: "empty-required-app.yaml", + expectedErr: nil, + }, + { + name: "invalid if required variable is omitted", + filename: "omitted-required-app.yaml", expectedErr: f.Ptr("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\""), }, { - name: "invalid app descriptor with a missing required variable among two", - filename: "mixed-required-app.yaml", + name: "invalid if required variable among two is omitted", + filename: "omitted-mixed-required-app.yaml", expectedErr: f.Ptr("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""), }, + { - name: "invalid app descriptor with not found brick id", + name: "invalid if brick id not found", filename: "not-found-brick-app.yaml", expectedErr: f.Ptr("brick \"arduino:not_existing_brick\" not found"), }, From 38b49bcc54cc2d9568929eb4ead9f0a2bef57726 Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 15:28:23 +0100 Subject: [PATCH 16/30] test: add test case for validation of non-existing variable in app descriptor --- .../app/testdata/validator/not-found-variable-app.yaml | 6 ++++++ internal/orchestrator/app/validator_test.go | 8 ++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 internal/orchestrator/app/testdata/validator/not-found-variable-app.yaml 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..b145f8a2 --- /dev/null +++ b/internal/orchestrator/app/testdata/validator/not-found-variable-app.yaml @@ -0,0 +1,6 @@ +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" \ No newline at end of file diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go index b40bfe4c..554871c4 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -43,16 +43,20 @@ func TestValidateAppDescriptorBricks(t *testing.T) { expectedErr: f.Ptr("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\""), }, { - name: "invalid if required variable among two is omitted", + name: "invalid if a required variable among two is omitted", filename: "omitted-mixed-required-app.yaml", expectedErr: f.Ptr("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""), }, - { name: "invalid if brick id not found", filename: "not-found-brick-app.yaml", expectedErr: f.Ptr("brick \"arduino:not_existing_brick\" not found"), }, + { + name: "invalid variable does not exist in the brick", + filename: "not-found-variable-app.yaml", + expectedErr: f.Ptr("variable \"NOT_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\""), + }, } for _, tc := range testCases { From 3708460a8e34889067638c209d25b335a30a0903 Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 15:53:42 +0100 Subject: [PATCH 17/30] fix: enhance brick validation to collect all errors and add new test cases for validation scenarios --- internal/orchestrator/app/parser.go | 18 ++++++++++++------ .../testdata/validator/all-required-app.yaml | 7 +++++++ .../validator/not-found-brick-app.yaml | 5 ++++- .../validator/not-found-variable-app.yaml | 4 +++- internal/orchestrator/app/validator_test.go | 9 +++++++-- 5 files changed, 33 insertions(+), 10 deletions(-) create mode 100644 internal/orchestrator/app/testdata/validator/all-required-app.yaml diff --git a/internal/orchestrator/app/parser.go b/internal/orchestrator/app/parser.go index abac997f..be085b84 100644 --- a/internal/orchestrator/app/parser.go +++ b/internal/orchestrator/app/parser.go @@ -140,35 +140,41 @@ func (a *AppDescriptor) IsValid() error { } // 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 returns an error if any brick is missing, -// if any variable referenced by the app does not exist in the corresponding brick, or if any required variable is missing. +// and that all required variables for each brick are present and valid. It collects and returns all validation +// errors found, 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 { - return fmt.Errorf("brick %q not found", appBrick.ID) + allErrors = errors.Join(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 { - return fmt.Errorf("variable %q does not exist on brick %q", appBrickName, indexBrick.ID) + 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 { - return fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID) + allErrors = errors.Join(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID)) } } } } - return nil + return allErrors } func isSingleEmoji(s string) bool { 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/not-found-brick-app.yaml b/internal/orchestrator/app/testdata/validator/not-found-brick-app.yaml index 6a688d81..59f76616 100644 --- a/internal/orchestrator/app/testdata/validator/not-found-brick-app.yaml +++ b/internal/orchestrator/app/testdata/validator/not-found-brick-app.yaml @@ -1,4 +1,7 @@ name: App no existing brick description: App no existing brick bricks: - - arduino:not_existing_brick \ No newline at end of file + - 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 index b145f8a2..44adafd2 100644 --- a/internal/orchestrator/app/testdata/validator/not-found-variable-app.yaml +++ b/internal/orchestrator/app/testdata/validator/not-found-variable-app.yaml @@ -3,4 +3,6 @@ description: App with non existing variable bricks: - arduino:arduino_cloud: variables: - NOT_EXISTING_VARIABLE: "this-is-a-not-existing-variable-for-the-brick" \ No newline at end of file + 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/validator_test.go b/internal/orchestrator/app/validator_test.go index 554871c4..449c4b22 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -22,6 +22,11 @@ func TestValidateAppDescriptorBricks(t *testing.T) { filename string expectedErr *string }{ + { + name: "valid with all required filled", + filename: "all-required-app.yaml", + expectedErr: nil, + }, { name: "valid with missing bricks", filename: "no-bricks-app.yaml", @@ -40,7 +45,7 @@ func TestValidateAppDescriptorBricks(t *testing.T) { { name: "invalid if required variable is omitted", filename: "omitted-required-app.yaml", - expectedErr: f.Ptr("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\""), + expectedErr: f.Ptr("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\"\nvariable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""), }, { name: "invalid if a required variable among two is omitted", @@ -53,7 +58,7 @@ func TestValidateAppDescriptorBricks(t *testing.T) { expectedErr: f.Ptr("brick \"arduino:not_existing_brick\" not found"), }, { - name: "invalid variable does not exist in the brick", + name: "invalid if variable does not exist in the brick", filename: "not-found-variable-app.yaml", expectedErr: f.Ptr("variable \"NOT_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\""), }, From d8028b6ba9495148bba8940c38e964d40d7da0b8 Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 16:17:05 +0100 Subject: [PATCH 18/30] fix: update ValidateBricks to return all validation errors as a slice and adjust related tests --- internal/orchestrator/app/parser.go | 12 ++-- internal/orchestrator/app/validator_test.go | 76 ++++++++++++--------- 2 files changed, 49 insertions(+), 39 deletions(-) diff --git a/internal/orchestrator/app/parser.go b/internal/orchestrator/app/parser.go index be085b84..19a8e799 100644 --- a/internal/orchestrator/app/parser.go +++ b/internal/orchestrator/app/parser.go @@ -141,19 +141,19 @@ func (a *AppDescriptor) IsValid() error { // 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, allowing the caller to see all issues at once rather than stopping at the first error. +// 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 { +func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) []error { if index == nil { return nil } - var allErrors error + 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)) + allErrors = append(allErrors, fmt.Errorf("brick %q not found", appBrick.ID)) continue // Skip further validation for this brick since it doesn't exist } @@ -161,7 +161,7 @@ func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) error { 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)) + allErrors = append(allErrors, fmt.Errorf("variable %q does not exist on brick %q", appBrickName, indexBrick.ID)) } } @@ -169,7 +169,7 @@ func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) error { 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)) + allErrors = append(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID)) } } } diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go index 449c4b22..19b4ed03 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -1,11 +1,11 @@ package app import ( + "errors" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.bug.st/f" "github.com/arduino/go-paths-helper" @@ -18,49 +18,58 @@ func TestValidateAppDescriptorBricks(t *testing.T) { require.NotNil(t, bricksIndex) testCases := []struct { - name string - filename string - expectedErr *string + name string + filename string + expectedErrors []error // Now expecting a slice of error messages }{ { - name: "valid with all required filled", - filename: "all-required-app.yaml", - expectedErr: nil, + name: "valid with all required filled", + filename: "all-required-app.yaml", + expectedErrors: nil, }, { - name: "valid with missing bricks", - filename: "no-bricks-app.yaml", - expectedErr: 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", - expectedErr: 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", - expectedErr: 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", - expectedErr: f.Ptr("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\"\nvariable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""), + 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", - expectedErr: f.Ptr("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", - expectedErr: f.Ptr("brick \"arduino:not_existing_brick\" not found"), + 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", - expectedErr: f.Ptr("variable \"NOT_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\""), + 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\""), + }, }, } @@ -69,12 +78,13 @@ func TestValidateAppDescriptorBricks(t *testing.T) { appDescriptor, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename)) require.NoError(t, err) - err = appDescriptor.ValidateBricks(bricksIndex) - if tc.expectedErr == nil { - assert.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.Error(t, err) - assert.Equal(t, *tc.expectedErr, err.Error()) + require.Len(t, errs, len(tc.expectedErrors), "Expected %d errors but got %d", len(tc.expectedErrors), len(errs)) + require.Exactly(t, tc.expectedErrors, errs) } }) } From 6a1743677c63e8b931979e2bb16d5acd3054d4e7 Mon Sep 17 00:00:00 2001 From: dido18 Date: Fri, 14 Nov 2025 16:44:08 +0100 Subject: [PATCH 19/30] fix: update error handling in ValidateBricks to yield all validation errors instead of the first one --- internal/orchestrator/app/parser.go | 3 ++- internal/orchestrator/orchestrator.go | 20 ++++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/internal/orchestrator/app/parser.go b/internal/orchestrator/app/parser.go index 19a8e799..c0d5e3c4 100644 --- a/internal/orchestrator/app/parser.go +++ b/internal/orchestrator/app/parser.go @@ -21,10 +21,11 @@ import ( "io" emoji "github.com/Andrew-M-C/go.emoji" - "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" "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 { diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index c6b0499d..37413640 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -122,9 +122,13 @@ func StartApp( ctx, cancel := context.WithCancel(ctx) defer cancel() - err := app.Descriptor.ValidateBricks(bricksIndex) - if err != nil { - yield(StreamMessage{error: err}) + errs := app.Descriptor.ValidateBricks(bricksIndex) + if errs != nil { + for _, e := range errs { + if !yield(StreamMessage{error: e}) { + return + } + } return } @@ -453,9 +457,13 @@ func RestartApp( ctx, cancel := context.WithCancel(ctx) defer cancel() - err := appToStart.Descriptor.ValidateBricks(bricksIndex) - if err != nil { - yield(StreamMessage{error: err}) + errs := appToStart.Descriptor.ValidateBricks(bricksIndex) + if errs != nil { + for _, e := range errs { + if !yield(StreamMessage{error: e}) { + return + } + } return } From c7a2e551bcf969358ec55c132f3f5f5652f98b48 Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 09:48:58 +0100 Subject: [PATCH 20/30] fix: refactor ValidateBricks to return a single error and update related tests for consistency --- internal/orchestrator/app/parser.go | 12 ++-- internal/orchestrator/app/validator_test.go | 69 +++++++++------------ internal/orchestrator/orchestrator.go | 20 ++---- 3 files changed, 43 insertions(+), 58 deletions(-) diff --git a/internal/orchestrator/app/parser.go b/internal/orchestrator/app/parser.go index c0d5e3c4..11c5390f 100644 --- a/internal/orchestrator/app/parser.go +++ b/internal/orchestrator/app/parser.go @@ -142,19 +142,19 @@ func (a *AppDescriptor) IsValid() error { // 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. +// 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 is nil, validation is skipped and nil is returned. -func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) []error { +func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) error { if index == nil { return nil } - var allErrors []error + 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)) + allErrors = errors.Join(allErrors, fmt.Errorf("brick %q not found", appBrick.ID)) continue // Skip further validation for this brick since it doesn't exist } @@ -162,7 +162,7 @@ func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) []error { 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)) + allErrors = errors.Join(allErrors, fmt.Errorf("variable %q does not exist on brick %q", appBrickName, indexBrick.ID)) } } @@ -170,7 +170,7 @@ func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) []error { 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)) + allErrors = errors.Join(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID)) } } } diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go index 19b4ed03..3beb859f 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -18,58 +18,52 @@ func TestValidateAppDescriptorBricks(t *testing.T) { require.NotNil(t, bricksIndex) testCases := []struct { - name string - filename string - expectedErrors []error // Now expecting a slice of error messages + name string + filename string + expectedError error }{ { - name: "valid with all required filled", - filename: "all-required-app.yaml", - expectedErrors: nil, + name: "valid with all required filled", + filename: "all-required-app.yaml", + expectedError: nil, }, { - name: "valid with missing bricks", - filename: "no-bricks-app.yaml", - expectedErrors: 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", - expectedErrors: 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", - expectedErrors: 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", - expectedErrors: []error{ + 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", - expectedErrors: []error{ - 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", - expectedErrors: []error{ - errors.New("brick \"arduino:not_existing_brick\" not found"), - }, + 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", - expectedErrors: []error{ - errors.New("variable \"NOT_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\""), - }, + 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\""), }, } @@ -78,13 +72,12 @@ func TestValidateAppDescriptorBricks(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") + err = appDescriptor.ValidateBricks(bricksIndex) + if tc.expectedError == nil { + assert.NoError(t, err, "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) + require.Error(t, err, "Expected validation error") + assert.Equal(t, tc.expectedError.Error(), err.Error(), "Error message should match") } }) } diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 37413640..c6b0499d 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -122,13 +122,9 @@ 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 - } - } + err := app.Descriptor.ValidateBricks(bricksIndex) + if err != nil { + yield(StreamMessage{error: err}) return } @@ -457,13 +453,9 @@ func RestartApp( 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 - } - } + err := appToStart.Descriptor.ValidateBricks(bricksIndex) + if err != nil { + yield(StreamMessage{error: err}) return } From 5742d452f2fea0a6e7e286d54aa33ceda55df651 Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 09:55:10 +0100 Subject: [PATCH 21/30] test: add dummy app configuration and main.py for brick creation tests --- internal/orchestrator/bricks/bricks_test.go | 53 +++++++------------ .../{AppFromExample => dummy-app}/app.yaml | 6 ++- .../python/main.py | 0 3 files changed, 24 insertions(+), 35 deletions(-) rename internal/orchestrator/bricks/testdata/{AppFromExample => dummy-app}/app.yaml (67%) rename internal/orchestrator/bricks/testdata/{AppFromExample => dummy-app}/python/main.py (100%) diff --git a/internal/orchestrator/bricks/bricks_test.go b/internal/orchestrator/bricks/bricks_test.go index f9974a42..cbd80854 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/internal/orchestrator/bricks/bricks_test.go @@ -16,9 +16,7 @@ package bricks import ( - "fmt" "testing" - "time" "github.com/arduino/go-paths-helper" "github.com/stretchr/testify/require" @@ -34,7 +32,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/AppFromExample"))) + err = brickService.BrickCreate(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", err.Error()) }) @@ -43,59 +41,56 @@ 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/AppFromExample"))) + err = brickService.BrickCreate(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: 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/AppFromExample"))) + 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()) }) - 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() - + t.Run("do not fail if a mandatory variable is not present", func(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(tempApp.String()))) - require.Nil(t, err) + err = brickService.BrickCreate(req, f.Must(app.Load("testdata/dummy-app"))) + require.NoError(t, err) - after, err := app.Load(tempApp.String()) + after, err := app.Load("testdata/dummy-app") 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) { - tempApp, cleanUp := copyToTempApp(t, paths.New("testdata/AppFromExample")) - defer cleanUp() + 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:dbstorage_sqlstore"} - err = brickService.BrickCreate(req, f.Must(app.Load(tempApp.String()))) + err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String()))) require.Nil(t, err) - after, err := app.Load(tempApp.String()) + after, err := app.Load(tempDummyApp.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) { - tempApp, cleanUp := copyToTempApp(t, paths.New("testdata/AppFromExample")) - defer cleanUp() - + 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) bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata")) require.Nil(t, err) brickService := NewService(nil, bricksIndex, nil) @@ -110,10 +105,10 @@ func TestBrickCreate(t *testing.T) { }, } - err = brickService.BrickCreate(req, f.Must(app.Load(tempApp.String()))) + err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String()))) require.Nil(t, err) - after, err := app.Load(tempApp.String()) + 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) @@ -122,14 +117,6 @@ 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/AppFromExample/app.yaml b/internal/orchestrator/bricks/testdata/dummy-app/app.yaml similarity index 67% rename from internal/orchestrator/bricks/testdata/AppFromExample/app.yaml rename to internal/orchestrator/bricks/testdata/dummy-app/app.yaml index 281821c6..ab215479 100644 --- a/internal/orchestrator/bricks/testdata/AppFromExample/app.yaml +++ b/internal/orchestrator/bricks/testdata/dummy-app/app.yaml @@ -1,6 +1,8 @@ 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: + variables: + ARDUINO_SECRET: a-secret-a +icon: ☁️ diff --git a/internal/orchestrator/bricks/testdata/AppFromExample/python/main.py b/internal/orchestrator/bricks/testdata/dummy-app/python/main.py similarity index 100% rename from internal/orchestrator/bricks/testdata/AppFromExample/python/main.py rename to internal/orchestrator/bricks/testdata/dummy-app/python/main.py From 1b18b36b34bdde2756b053f91a9ab0ba4e66c3ad Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 09:58:45 +0100 Subject: [PATCH 22/30] fix: add logging for missing required variables in BrickCreate function --- internal/orchestrator/bricks/bricks.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/orchestrator/bricks/bricks.go b/internal/orchestrator/bricks/bricks.go index 50459be1..dc59d941 100644 --- a/internal/orchestrator/bricks/bricks.go +++ b/internal/orchestrator/bricks/bricks.go @@ -290,6 +290,7 @@ func (s *Service) BrickCreate( for _, brickVar := range brick.Variables { if brickVar.DefaultValue == "" { if _, exist := req.Variables[brickVar.Name]; !exist { + // 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) } } From 36319aabf2379a8d221c1d7cc2bb0cd2b409bd2d Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 10:00:31 +0100 Subject: [PATCH 23/30] chore: remove unused app.golden.yaml test data file --- internal/orchestrator/bricks/testdata/app.golden.yaml | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 internal/orchestrator/bricks/testdata/app.golden.yaml diff --git a/internal/orchestrator/bricks/testdata/app.golden.yaml b/internal/orchestrator/bricks/testdata/app.golden.yaml deleted file mode 100644 index e69de29b..00000000 From d9992fe95729904f04b47c9d7d544f8a334c127f Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 10:02:05 +0100 Subject: [PATCH 24/30] fix: clean up app.yaml by removing unused variables section --- internal/orchestrator/bricks/testdata/dummy-app/app.yaml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/orchestrator/bricks/testdata/dummy-app/app.yaml b/internal/orchestrator/bricks/testdata/dummy-app/app.yaml index ab215479..281821c6 100644 --- a/internal/orchestrator/bricks/testdata/dummy-app/app.yaml +++ b/internal/orchestrator/bricks/testdata/dummy-app/app.yaml @@ -1,8 +1,6 @@ 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: - variables: - ARDUINO_SECRET: a-secret-a -icon: ☁️ +- arduino:arduino_cloud: \ No newline at end of file From 89bb0b99d588f31ee81e9f2acd47c32d271dffdc Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 10:15:25 +0100 Subject: [PATCH 25/30] refactor: move ValidateBricks function to a new file and update references --- internal/orchestrator/app/parser.go | 40 ----------------- internal/orchestrator/app/validator.go | 44 +++++++++++++++++++ internal/orchestrator/app/validator_test.go | 2 +- .../bricks/testdata/dummy-app/app.yaml | 6 ++- internal/orchestrator/orchestrator.go | 31 ++++++------- 5 files changed, 65 insertions(+), 58 deletions(-) create mode 100644 internal/orchestrator/app/validator.go diff --git a/internal/orchestrator/app/parser.go b/internal/orchestrator/app/parser.go index 11c5390f..4bff1ba1 100644 --- a/internal/orchestrator/app/parser.go +++ b/internal/orchestrator/app/parser.go @@ -24,8 +24,6 @@ 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 { @@ -140,44 +138,6 @@ 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 as a single joined error, 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 = errors.Join(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 = 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 -} - func isSingleEmoji(s string) bool { emojis := 0 for it := emoji.IterateChars(s); it.Next(); { 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 index 3beb859f..2c10499b 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -72,7 +72,7 @@ func TestValidateAppDescriptorBricks(t *testing.T) { appDescriptor, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename)) require.NoError(t, err) - err = appDescriptor.ValidateBricks(bricksIndex) + err = ValidateBricks(appDescriptor, bricksIndex) if tc.expectedError == nil { assert.NoError(t, err, "Expected no validation errors") } else { diff --git a/internal/orchestrator/bricks/testdata/dummy-app/app.yaml b/internal/orchestrator/bricks/testdata/dummy-app/app.yaml index 281821c6..ab215479 100644 --- a/internal/orchestrator/bricks/testdata/dummy-app/app.yaml +++ b/internal/orchestrator/bricks/testdata/dummy-app/app.yaml @@ -1,6 +1,8 @@ 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: + variables: + ARDUINO_SECRET: a-secret-a +icon: ☁️ diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index c6b0499d..f027570e 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -44,6 +44,7 @@ import ( "github.com/arduino/arduino-app-cli/internal/helpers" "github.com/arduino/arduino-app-cli/internal/micro" "github.com/arduino/arduino-app-cli/internal/orchestrator/app" + appspecification "github.com/arduino/arduino-app-cli/internal/orchestrator/app" appgenerator "github.com/arduino/arduino-app-cli/internal/orchestrator/app/generator" "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" "github.com/arduino/arduino-app-cli/internal/orchestrator/config" @@ -114,7 +115,7 @@ func StartApp( provisioner *Provision, modelsIndex *modelsindex.ModelsIndex, bricksIndex *bricksindex.BricksIndex, - app app.ArduinoApp, + app appspecification.ArduinoApp, cfg config.Configuration, staticStore *store.StaticStore, ) iter.Seq[StreamMessage] { @@ -122,7 +123,7 @@ func StartApp( ctx, cancel := context.WithCancel(ctx) defer cancel() - err := app.Descriptor.ValidateBricks(bricksIndex) + err := appspecification.ValidateBricks(app.Descriptor, bricksIndex) if err != nil { yield(StreamMessage{error: err}) return @@ -255,7 +256,7 @@ func StartApp( // - model configuration variables (variables defined in the model configuration) // - brick instance variables (variables defined in the app.yaml for the brick instance) // In addition, it adds some useful environment variables like APP_HOME and HOST_IP. -func getAppEnvironmentVariables(app app.ArduinoApp, brickIndex *bricksindex.BricksIndex, modelsIndex *modelsindex.ModelsIndex) helpers.EnvVars { +func getAppEnvironmentVariables(app appspecification.ArduinoApp, brickIndex *bricksindex.BricksIndex, modelsIndex *modelsindex.ModelsIndex) helpers.EnvVars { envs := make(helpers.EnvVars) for _, brick := range app.Descriptor.Bricks { @@ -383,7 +384,7 @@ func getVideoDevices() map[int]string { return deviceMap } -func stopAppWithCmd(ctx context.Context, app app.ArduinoApp, cmd string) iter.Seq[StreamMessage] { +func stopAppWithCmd(ctx context.Context, app appspecification.ArduinoApp, cmd string) iter.Seq[StreamMessage] { return func(yield func(StreamMessage) bool) { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -431,11 +432,11 @@ func stopAppWithCmd(ctx context.Context, app app.ArduinoApp, cmd string) iter.Se } } -func StopApp(ctx context.Context, app app.ArduinoApp) iter.Seq[StreamMessage] { +func StopApp(ctx context.Context, app appspecification.ArduinoApp) iter.Seq[StreamMessage] { return stopAppWithCmd(ctx, app, "stop") } -func StopAndDestroyApp(ctx context.Context, app app.ArduinoApp) iter.Seq[StreamMessage] { +func StopAndDestroyApp(ctx context.Context, app appspecification.ArduinoApp) iter.Seq[StreamMessage] { return stopAppWithCmd(ctx, app, "down") } @@ -445,7 +446,7 @@ func RestartApp( provisioner *Provision, modelsIndex *modelsindex.ModelsIndex, bricksIndex *bricksindex.BricksIndex, - appToStart app.ArduinoApp, + appToStart appspecification.ArduinoApp, cfg config.Configuration, staticStore *store.StaticStore, ) iter.Seq[StreamMessage] { @@ -453,7 +454,7 @@ func RestartApp( ctx, cancel := context.WithCancel(ctx) defer cancel() - err := appToStart.Descriptor.ValidateBricks(bricksIndex) + err := appspecification.ValidateBricks(appToStart.Descriptor, bricksIndex) if err != nil { yield(StreamMessage{error: err}) return @@ -677,7 +678,7 @@ type AppDetailedBrick struct { func AppDetails( ctx context.Context, docker command.Cli, - userApp app.ArduinoApp, + userApp appspecification.ArduinoApp, bricksIndex *bricksindex.BricksIndex, idProvider *app.IDProvider, cfg config.Configuration, @@ -901,7 +902,7 @@ func CloneApp( return CloneAppResponse{ID: id}, nil } -func DeleteApp(ctx context.Context, app app.ArduinoApp) error { +func DeleteApp(ctx context.Context, app appspecification.ArduinoApp) error { for msg := range StopApp(ctx, app) { if msg.error != nil { return fmt.Errorf("failed to stop app: %w", msg.error) @@ -912,7 +913,7 @@ func DeleteApp(ctx context.Context, app app.ArduinoApp) error { const defaultAppFileName = "default.app" -func SetDefaultApp(app *app.ArduinoApp, cfg config.Configuration) error { +func SetDefaultApp(app *appspecification.ArduinoApp, cfg config.Configuration) error { defaultAppPath := cfg.DataDir().Join(defaultAppFileName) // Remove the default app file if the app is nil. @@ -927,7 +928,7 @@ func SetDefaultApp(app *app.ArduinoApp, cfg config.Configuration) error { return fatomic.WriteFile(defaultAppPath.String(), []byte(app.FullPath.String()), os.FileMode(0644)) } -func GetDefaultApp(cfg config.Configuration) (*app.ArduinoApp, error) { +func GetDefaultApp(cfg config.Configuration) (*appspecification.ArduinoApp, error) { defaultAppFilePath := cfg.DataDir().Join(defaultAppFileName) if !defaultAppFilePath.Exist() { return nil, nil @@ -965,7 +966,7 @@ type AppEditRequest struct { func EditApp( req AppEditRequest, - editApp *app.ArduinoApp, + editApp *appspecification.ArduinoApp, cfg config.Configuration, ) (editErr error) { if req.Default != nil { @@ -1005,7 +1006,7 @@ func EditApp( return nil } -func editAppDefaults(userApp *app.ArduinoApp, isDefault bool, cfg config.Configuration) error { +func editAppDefaults(userApp *appspecification.ArduinoApp, isDefault bool, cfg config.Configuration) error { if isDefault { if err := SetDefaultApp(userApp, cfg); err != nil { return fmt.Errorf("failed to set default app: %w", err) @@ -1134,7 +1135,7 @@ func addLedControl(volumes []volume) []volume { func compileUploadSketch( ctx context.Context, - arduinoApp *app.ArduinoApp, + arduinoApp *appspecification.ArduinoApp, w io.Writer, ) error { logrus.SetLevel(logrus.ErrorLevel) // Reduce the log level of arduino-cli From 8acb1cc6aa969e1043389139230bb462a724f6b1 Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 10:20:41 +0100 Subject: [PATCH 26/30] refactor: update app type references from appspecification to app --- internal/orchestrator/orchestrator.go | 49 +++++++++++++-------------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index f027570e..e86c942c 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -44,7 +44,6 @@ import ( "github.com/arduino/arduino-app-cli/internal/helpers" "github.com/arduino/arduino-app-cli/internal/micro" "github.com/arduino/arduino-app-cli/internal/orchestrator/app" - appspecification "github.com/arduino/arduino-app-cli/internal/orchestrator/app" appgenerator "github.com/arduino/arduino-app-cli/internal/orchestrator/app/generator" "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" "github.com/arduino/arduino-app-cli/internal/orchestrator/config" @@ -115,7 +114,7 @@ func StartApp( provisioner *Provision, modelsIndex *modelsindex.ModelsIndex, bricksIndex *bricksindex.BricksIndex, - app appspecification.ArduinoApp, + appToStart app.ArduinoApp, cfg config.Configuration, staticStore *store.StaticStore, ) iter.Seq[StreamMessage] { @@ -123,7 +122,7 @@ func StartApp( ctx, cancel := context.WithCancel(ctx) defer cancel() - err := appspecification.ValidateBricks(app.Descriptor, bricksIndex) + err := app.ValidateBricks(appToStart.Descriptor, bricksIndex) if err != nil { yield(StreamMessage{error: err}) return @@ -138,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 } @@ -155,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 } @@ -168,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 } @@ -184,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 } @@ -195,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()) } @@ -256,7 +255,7 @@ func StartApp( // - model configuration variables (variables defined in the model configuration) // - brick instance variables (variables defined in the app.yaml for the brick instance) // In addition, it adds some useful environment variables like APP_HOME and HOST_IP. -func getAppEnvironmentVariables(app appspecification.ArduinoApp, brickIndex *bricksindex.BricksIndex, modelsIndex *modelsindex.ModelsIndex) helpers.EnvVars { +func getAppEnvironmentVariables(app app.ArduinoApp, brickIndex *bricksindex.BricksIndex, modelsIndex *modelsindex.ModelsIndex) helpers.EnvVars { envs := make(helpers.EnvVars) for _, brick := range app.Descriptor.Bricks { @@ -384,7 +383,7 @@ func getVideoDevices() map[int]string { return deviceMap } -func stopAppWithCmd(ctx context.Context, app appspecification.ArduinoApp, cmd string) iter.Seq[StreamMessage] { +func stopAppWithCmd(ctx context.Context, app app.ArduinoApp, cmd string) iter.Seq[StreamMessage] { return func(yield func(StreamMessage) bool) { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -432,11 +431,11 @@ func stopAppWithCmd(ctx context.Context, app appspecification.ArduinoApp, cmd st } } -func StopApp(ctx context.Context, app appspecification.ArduinoApp) iter.Seq[StreamMessage] { +func StopApp(ctx context.Context, app app.ArduinoApp) iter.Seq[StreamMessage] { return stopAppWithCmd(ctx, app, "stop") } -func StopAndDestroyApp(ctx context.Context, app appspecification.ArduinoApp) iter.Seq[StreamMessage] { +func StopAndDestroyApp(ctx context.Context, app app.ArduinoApp) iter.Seq[StreamMessage] { return stopAppWithCmd(ctx, app, "down") } @@ -446,7 +445,7 @@ func RestartApp( provisioner *Provision, modelsIndex *modelsindex.ModelsIndex, bricksIndex *bricksindex.BricksIndex, - appToStart appspecification.ArduinoApp, + appToStart app.ArduinoApp, cfg config.Configuration, staticStore *store.StaticStore, ) iter.Seq[StreamMessage] { @@ -454,7 +453,7 @@ func RestartApp( ctx, cancel := context.WithCancel(ctx) defer cancel() - err := appspecification.ValidateBricks(appToStart.Descriptor, bricksIndex) + err := app.ValidateBricks(appToStart.Descriptor, bricksIndex) if err != nil { yield(StreamMessage{error: err}) return @@ -678,7 +677,7 @@ type AppDetailedBrick struct { func AppDetails( ctx context.Context, docker command.Cli, - userApp appspecification.ArduinoApp, + userApp app.ArduinoApp, bricksIndex *bricksindex.BricksIndex, idProvider *app.IDProvider, cfg config.Configuration, @@ -902,7 +901,7 @@ func CloneApp( return CloneAppResponse{ID: id}, nil } -func DeleteApp(ctx context.Context, app appspecification.ArduinoApp) error { +func DeleteApp(ctx context.Context, app app.ArduinoApp) error { for msg := range StopApp(ctx, app) { if msg.error != nil { return fmt.Errorf("failed to stop app: %w", msg.error) @@ -913,7 +912,7 @@ func DeleteApp(ctx context.Context, app appspecification.ArduinoApp) error { const defaultAppFileName = "default.app" -func SetDefaultApp(app *appspecification.ArduinoApp, cfg config.Configuration) error { +func SetDefaultApp(app *app.ArduinoApp, cfg config.Configuration) error { defaultAppPath := cfg.DataDir().Join(defaultAppFileName) // Remove the default app file if the app is nil. @@ -928,7 +927,7 @@ func SetDefaultApp(app *appspecification.ArduinoApp, cfg config.Configuration) e return fatomic.WriteFile(defaultAppPath.String(), []byte(app.FullPath.String()), os.FileMode(0644)) } -func GetDefaultApp(cfg config.Configuration) (*appspecification.ArduinoApp, error) { +func GetDefaultApp(cfg config.Configuration) (*app.ArduinoApp, error) { defaultAppFilePath := cfg.DataDir().Join(defaultAppFileName) if !defaultAppFilePath.Exist() { return nil, nil @@ -966,7 +965,7 @@ type AppEditRequest struct { func EditApp( req AppEditRequest, - editApp *appspecification.ArduinoApp, + editApp *app.ArduinoApp, cfg config.Configuration, ) (editErr error) { if req.Default != nil { @@ -1006,7 +1005,7 @@ func EditApp( return nil } -func editAppDefaults(userApp *appspecification.ArduinoApp, isDefault bool, cfg config.Configuration) error { +func editAppDefaults(userApp *app.ArduinoApp, isDefault bool, cfg config.Configuration) error { if isDefault { if err := SetDefaultApp(userApp, cfg); err != nil { return fmt.Errorf("failed to set default app: %w", err) @@ -1135,7 +1134,7 @@ func addLedControl(volumes []volume) []volume { func compileUploadSketch( ctx context.Context, - arduinoApp *appspecification.ArduinoApp, + arduinoApp *app.ArduinoApp, w io.Writer, ) error { logrus.SetLevel(logrus.ErrorLevel) // Reduce the log level of arduino-cli From 4b525b35860338d16a0656740b3d07c17637f688 Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 10:24:18 +0100 Subject: [PATCH 27/30] FIX --- internal/orchestrator/bricks/bricks_test.go | 9 +++++++-- internal/orchestrator/bricks/testdata/dummy-app/app.yaml | 6 ++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/internal/orchestrator/bricks/bricks_test.go b/internal/orchestrator/bricks/bricks_test.go index cbd80854..bfb5cb75 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/internal/orchestrator/bricks/bricks_test.go @@ -57,13 +57,18 @@ func TestBrickCreate(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"))) + err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String()))) require.NoError(t, err) - after, err := app.Load("testdata/dummy-app") + 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) diff --git a/internal/orchestrator/bricks/testdata/dummy-app/app.yaml b/internal/orchestrator/bricks/testdata/dummy-app/app.yaml index ab215479..281821c6 100644 --- a/internal/orchestrator/bricks/testdata/dummy-app/app.yaml +++ b/internal/orchestrator/bricks/testdata/dummy-app/app.yaml @@ -1,8 +1,6 @@ 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: - variables: - ARDUINO_SECRET: a-secret-a -icon: ☁️ +- arduino:arduino_cloud: \ No newline at end of file From 7a9bdf2e1b8d6b21ced3cfa9b1193e94a3f127c4 Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 11:06:06 +0100 Subject: [PATCH 28/30] refactor: log a warning for undeclared variables in brick configuration --- internal/orchestrator/app/validator.go | 3 ++- internal/orchestrator/app/validator_test.go | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/orchestrator/app/validator.go b/internal/orchestrator/app/validator.go index aa184a19..e8986faa 100644 --- a/internal/orchestrator/app/validator.go +++ b/internal/orchestrator/app/validator.go @@ -3,6 +3,7 @@ package app import ( "errors" "fmt" + "log/slog" "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" ) @@ -27,7 +28,7 @@ func ValidateBricks(a AppDescriptor, index *bricksindex.BricksIndex) error { 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)) + slog.Warn("variable is referenced but not declared in the brick configuration", "variable", appBrickName, "brick", indexBrick.ID) } } diff --git a/internal/orchestrator/app/validator_test.go b/internal/orchestrator/app/validator_test.go index 2c10499b..fd10d783 100644 --- a/internal/orchestrator/app/validator_test.go +++ b/internal/orchestrator/app/validator_test.go @@ -61,9 +61,9 @@ func TestValidateAppDescriptorBricks(t *testing.T) { expectedError: errors.New("brick \"arduino:not_existing_brick\" not found"), }, { - name: "invalid if variable does not exist in the brick", + name: "log a warning 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\""), + expectedError: nil, }, } From 7f935b96a558426c33a26cc9ed111ee75a70feb7 Mon Sep 17 00:00:00 2001 From: Davide Date: Mon, 17 Nov 2025 11:59:22 +0100 Subject: [PATCH 29/30] Update internal/orchestrator/app/validator.go Co-authored-by: mirkoCrobu <214636120+mirkoCrobu@users.noreply.github.com> --- internal/orchestrator/app/validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/orchestrator/app/validator.go b/internal/orchestrator/app/validator.go index e8986faa..268622b4 100644 --- a/internal/orchestrator/app/validator.go +++ b/internal/orchestrator/app/validator.go @@ -9,7 +9,7 @@ import ( ) // 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. +// 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. // 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 { From 1661442c7c414a8797459de240ddf319adc7024e Mon Sep 17 00:00:00 2001 From: dido18 Date: Mon, 17 Nov 2025 12:00:26 +0100 Subject: [PATCH 30/30] refactor: rename variable for clarity in ValidateBricks function --- internal/orchestrator/app/validator.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/orchestrator/app/validator.go b/internal/orchestrator/app/validator.go index 268622b4..9da76220 100644 --- a/internal/orchestrator/app/validator.go +++ b/internal/orchestrator/app/validator.go @@ -25,10 +25,10 @@ func ValidateBricks(a AppDescriptor, index *bricksindex.BricksIndex) error { continue // Skip further validation for this brick since it doesn't exist } - for appBrickName := range appBrick.Variables { - _, exist := indexBrick.GetVariable(appBrickName) + for appBrickVariableName := range appBrick.Variables { + _, exist := indexBrick.GetVariable(appBrickVariableName) if !exist { - slog.Warn("variable is referenced but not declared in the brick configuration", "variable", appBrickName, "brick", indexBrick.ID) + slog.Warn("variable is referenced but not declared in the brick configuration", "variable", appBrickVariableName, "brick", indexBrick.ID) } }