diff --git a/internal/api/docs/openapi.yaml b/internal/api/docs/openapi.yaml index 577d77d1..f2b3a999 100644 --- a/internal/api/docs/openapi.yaml +++ b/internal/api/docs/openapi.yaml @@ -1276,6 +1276,17 @@ components: name: type: string type: object + BrickConfigVariable: + properties: + description: + type: string + name: + type: string + required: + type: boolean + value: + type: string + type: object BrickCreateUpdateRequest: properties: model: @@ -1325,6 +1336,10 @@ components: type: string category: type: string + config_variables: + items: + $ref: '#/components/schemas/BrickConfigVariable' + type: array id: type: string model: @@ -1336,6 +1351,8 @@ components: variables: additionalProperties: type: string + description: 'Deprecated: use config_variables instead. This field is kept + for backward compatibility.' type: object type: object BrickListItem: diff --git a/internal/e2e/client/client.gen.go b/internal/e2e/client/client.gen.go index 9e12c9ec..f6094430 100644 --- a/internal/e2e/client/client.gen.go +++ b/internal/e2e/client/client.gen.go @@ -119,6 +119,14 @@ type AppReference struct { Name *string `json:"name,omitempty"` } +// BrickConfigVariable defines model for BrickConfigVariable. +type BrickConfigVariable struct { + Description *string `json:"description,omitempty"` + Name *string `json:"name,omitempty"` + Required *bool `json:"required,omitempty"` + Value *string `json:"value,omitempty"` +} + // BrickCreateUpdateRequest defines model for BrickCreateUpdateRequest. type BrickCreateUpdateRequest struct { Model *string `json:"model"` @@ -142,12 +150,15 @@ type BrickDetailsResult struct { // BrickInstance defines model for BrickInstance. type BrickInstance struct { - Author *string `json:"author,omitempty"` - Category *string `json:"category,omitempty"` - Id *string `json:"id,omitempty"` - Model *string `json:"model,omitempty"` - Name *string `json:"name,omitempty"` - Status *string `json:"status,omitempty"` + Author *string `json:"author,omitempty"` + Category *string `json:"category,omitempty"` + ConfigVariables *[]BrickConfigVariable `json:"config_variables,omitempty"` + Id *string `json:"id,omitempty"` + Model *string `json:"model,omitempty"` + Name *string `json:"name,omitempty"` + Status *string `json:"status,omitempty"` + + // Variables Deprecated: use config_variables instead. This field is kept for backward compatibility. Variables *map[string]string `json:"variables,omitempty"` } diff --git a/internal/e2e/daemon/app_test.go b/internal/e2e/daemon/app_test.go index bc6556e6..1de8ff64 100644 --- a/internal/e2e/daemon/app_test.go +++ b/internal/e2e/daemon/app_test.go @@ -470,7 +470,7 @@ func TestDeleteApp(t *testing.T) { t.Run("DeletingExampleApp_Fail", func(t *testing.T) { var actualResponseBody models.ErrorResponse - deleteResp, err := httpClient.DeleteApp(t.Context(), noExisitingExample) + deleteResp, err := httpClient.DeleteApp(t.Context(), "ZXhhbXBsZXM6anVzdGJsaW5f") require.NoError(t, err) defer deleteResp.Body.Close() @@ -818,7 +818,7 @@ func TestAppPorts(t *testing.T) { respBrick, err := httpClient.UpsertAppBrickInstanceWithResponse( t.Context(), *createResp.JSON201.Id, - StreamLitUi, + "arduino:streamlit_ui", client.BrickCreateUpdateRequest{}, func(ctx context.Context, req *http.Request) error { return nil }, ) diff --git a/internal/e2e/daemon/const.go b/internal/e2e/daemon/const.go index f656fd0f..3bb35899 100644 --- a/internal/e2e/daemon/const.go +++ b/internal/e2e/daemon/const.go @@ -16,11 +16,7 @@ package daemon const ( - ImageClassifactionBrickID = "arduino:image_classification" - StreamLitUi = "arduino:streamlit_ui" - expectedDetailsAppNotfound = "unable to find the app" - expectedDetailsAppInvalidAppId = "invalid app id" - noExistingApp = "dXNlcjp0ZXN0LWFwcAw" - malformedAppId = "this-is-definitely-not-base64" - noExisitingExample = "ZXhhbXBsZXM6anVzdGJsaW5f" + ImageClassifactionBrickID = "arduino:image_classification" + noExistingApp = "dXNlcjp0ZXN0LWFwcAw" + malformedAppId = "this-is-definitely-not-base64" ) diff --git a/internal/e2e/daemon/instance_bricks_test.go b/internal/e2e/daemon/instance_bricks_test.go index 3399476c..c210a9e6 100644 --- a/internal/e2e/daemon/instance_bricks_test.go +++ b/internal/e2e/daemon/instance_bricks_test.go @@ -31,6 +31,28 @@ import ( "github.com/arduino/arduino-app-cli/internal/e2e/client" ) +const ( + expectedDetailsAppInvalidAppId = "invalid app id" + expectedDetailsAppNotfound = "unable to find the app" +) + +var ( + expectedConfigVariables = []client.BrickConfigVariable{ + { + Description: f.Ptr("path to the custom model directory"), + Name: f.Ptr("CUSTOM_MODEL_PATH"), + Required: f.Ptr(false), + Value: f.Ptr("/home/arduino/.arduino-bricks/ei-models"), + }, + { + Description: f.Ptr("path to the model file"), + Name: f.Ptr("EI_CLASSIFICATION_MODEL"), + Required: f.Ptr(false), + Value: f.Ptr("/models/ootb/ei/mobilenet-v2-224px.eim"), + }, + } +) + func setupTestApp(t *testing.T) (*client.CreateAppResp, *client.ClientWithResponses) { httpClient := GetHttpclient(t) createResp, err := httpClient.CreateAppWithResponse( @@ -68,6 +90,7 @@ func TestGetAppBrickInstances(t *testing.T) { require.NoError(t, err) require.Len(t, *brickInstances.JSON200.Bricks, 1) require.Equal(t, ImageClassifactionBrickID, *(*brickInstances.JSON200.Bricks)[0].Id) + require.Equal(t, expectedConfigVariables, *(*brickInstances.JSON200.Bricks)[0].ConfigVariables) }) @@ -111,6 +134,7 @@ func TestGetAppBrickInstanceById(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, brickInstance.JSON200) require.Equal(t, ImageClassifactionBrickID, *brickInstance.JSON200.Id) + require.Equal(t, expectedConfigVariables, (*brickInstance.JSON200.ConfigVariables)) }) t.Run("GetAppBrickInstanceByBrickID_InvalidAppID_Fails", func(t *testing.T) { diff --git a/internal/orchestrator/bricks/bricks.go b/internal/orchestrator/bricks/bricks.go index 20dd9948..e8dea9da 100644 --- a/internal/orchestrator/bricks/bricks.go +++ b/internal/orchestrator/bricks/bricks.go @@ -19,7 +19,6 @@ import ( "errors" "fmt" "log/slog" - "maps" "slices" "github.com/arduino/go-paths-helper" @@ -80,15 +79,20 @@ func (s *Service) AppBrickInstancesList(a *app.ArduinoApp) (AppBrickInstancesRes if !found { return AppBrickInstancesResult{}, fmt.Errorf("brick not found with id %s", brickInstance.ID) } + + variablesMap, configVariables := getBrickConfigDetails(brick, brickInstance.Variables) + res.BrickInstances[i] = BrickInstance{ - ID: brick.ID, - Name: brick.Name, - Author: "Arduino", // TODO: for now we only support our bricks - Category: brick.Category, - Status: "installed", - ModelID: brickInstance.Model, // TODO: in case is not set by the user, should we return the default model? - Variables: brickInstance.Variables, // TODO: do we want to show also the default value of not explicitly set variables? + ID: brick.ID, + Name: brick.Name, + Author: "Arduino", // TODO: for now we only support our bricks + Category: brick.Category, + Status: "installed", + ModelID: brickInstance.Model, // TODO: in case is not set by the user, should we return the default model? + Variables: variablesMap, // TODO: do we want to show also the default value of not explicitly set variables? + ConfigVariables: configVariables, } + } return res, nil } @@ -104,12 +108,7 @@ func (s *Service) AppBrickInstanceDetails(a *app.ArduinoApp, brickID string) (Br return BrickInstance{}, fmt.Errorf("brick %s not added in the app", brickID) } - variables := make(map[string]string, len(brick.Variables)) - for _, v := range brick.Variables { - variables[v.Name] = v.DefaultValue - } - // Add/Update the variables with the ones from the app descriptor - maps.Copy(variables, a.Descriptor.Bricks[brickIndex].Variables) + variables, configVariables := getBrickConfigDetails(brick, a.Descriptor.Bricks[brickIndex].Variables) modelID := a.Descriptor.Bricks[brickIndex].Model if modelID == "" { @@ -117,16 +116,43 @@ func (s *Service) AppBrickInstanceDetails(a *app.ArduinoApp, brickID string) (Br } return BrickInstance{ - ID: brickID, - Name: brick.Name, - Author: "Arduino", // TODO: for now we only support our bricks - Category: brick.Category, - Status: "installed", // For now every Arduino brick are installed - Variables: variables, - ModelID: modelID, + ID: brickID, + Name: brick.Name, + Author: "Arduino", // TODO: for now we only support our bricks + Category: brick.Category, + Status: "installed", // For now every Arduino brick are installed + Variables: variables, + ConfigVariables: configVariables, + ModelID: modelID, }, nil } +func getBrickConfigDetails( + brick *bricksindex.Brick, userVariables map[string]string, +) (map[string]string, []BrickConfigVariable) { + variablesMap := make(map[string]string, len(brick.Variables)) + variableDetails := make([]BrickConfigVariable, 0, len(brick.Variables)) + + for _, v := range brick.Variables { + finalValue := v.DefaultValue + + userValue, ok := userVariables[v.Name] + if ok { + finalValue = userValue + } + variablesMap[v.Name] = finalValue + + variableDetails = append(variableDetails, BrickConfigVariable{ + Name: v.Name, + Value: finalValue, + Description: v.Description, + Required: v.IsRequired(), + }) + } + + return variablesMap, variableDetails +} + func (s *Service) BricksDetails(id string, idProvider *app.IDProvider, cfg config.Configuration) (BrickDetailsResult, error) { brick, found := s.bricksIndex.FindBrickByID(id) diff --git a/internal/orchestrator/bricks/bricks_test.go b/internal/orchestrator/bricks/bricks_test.go index b5a93173..2f03d96b 100644 --- a/internal/orchestrator/bricks/bricks_test.go +++ b/internal/orchestrator/bricks/bricks_test.go @@ -110,3 +110,83 @@ func TestBrickCreate(t *testing.T) { require.Equal(t, secret, after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"]) }) } + +func TestGetBrickInstanceVariableDetails(t *testing.T) { + tests := []struct { + name string + brick *bricksindex.Brick + userVariables map[string]string + expectedConfigVariables []BrickConfigVariable + expectedVariableMap map[string]string + }{ + { + name: "variable is present in the map", + brick: &bricksindex.Brick{ + Variables: []bricksindex.BrickVariable{ + {Name: "VAR1", Description: "desc"}, + }, + }, + userVariables: map[string]string{"VAR1": "value1"}, + expectedConfigVariables: []BrickConfigVariable{ + {Name: "VAR1", Value: "value1", Description: "desc", Required: true}, + }, + expectedVariableMap: map[string]string{"VAR1": "value1"}, + }, + { + name: "variable not present in the map", + brick: &bricksindex.Brick{ + Variables: []bricksindex.BrickVariable{ + {Name: "VAR1", Description: "desc"}, + }, + }, + userVariables: map[string]string{}, + expectedConfigVariables: []BrickConfigVariable{ + {Name: "VAR1", Value: "", Description: "desc", Required: true}, + }, + expectedVariableMap: map[string]string{"VAR1": ""}, + }, + { + name: "variable with default value", + brick: &bricksindex.Brick{ + Variables: []bricksindex.BrickVariable{ + {Name: "VAR1", DefaultValue: "default", Description: "desc"}, + }, + }, + userVariables: map[string]string{}, + expectedConfigVariables: []BrickConfigVariable{ + {Name: "VAR1", Value: "default", Description: "desc", Required: false}, + }, + expectedVariableMap: map[string]string{"VAR1": "default"}, + }, + { + name: "multiple variables", + brick: &bricksindex.Brick{ + Variables: []bricksindex.BrickVariable{ + {Name: "VAR1", Description: "desc1"}, + {Name: "VAR2", DefaultValue: "def2", Description: "desc2"}, + }, + }, + userVariables: map[string]string{"VAR1": "v1"}, + expectedConfigVariables: []BrickConfigVariable{ + {Name: "VAR1", Value: "v1", Description: "desc1", Required: true}, + {Name: "VAR2", Value: "def2", Description: "desc2", Required: false}, + }, + expectedVariableMap: map[string]string{"VAR1": "v1", "VAR2": "def2"}, + }, + { + name: "no variables", + brick: &bricksindex.Brick{Variables: []bricksindex.BrickVariable{}}, + userVariables: map[string]string{}, + expectedConfigVariables: []BrickConfigVariable{}, + expectedVariableMap: map[string]string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + actualVariableMap, actualConfigVariables := getBrickConfigDetails(tt.brick, tt.userVariables) + require.Equal(t, tt.expectedVariableMap, actualVariableMap) + require.Equal(t, tt.expectedConfigVariables, actualConfigVariables) + }) + } +} diff --git a/internal/orchestrator/bricks/types.go b/internal/orchestrator/bricks/types.go index ae803745..868c563a 100644 --- a/internal/orchestrator/bricks/types.go +++ b/internal/orchestrator/bricks/types.go @@ -34,13 +34,21 @@ type AppBrickInstancesResult struct { } type BrickInstance struct { - ID string `json:"id"` - Name string `json:"name"` - Author string `json:"author"` - Category string `json:"category"` - Status string `json:"status"` - Variables map[string]string `json:"variables,omitempty"` - ModelID string `json:"model,omitempty"` + ID string `json:"id"` + Name string `json:"name"` + Author string `json:"author"` + Category string `json:"category"` + Status string `json:"status"` + Variables map[string]string `json:"variables,omitempty" description:"Deprecated: use config_variables instead. This field is kept for backward compatibility."` + ConfigVariables []BrickConfigVariable `json:"config_variables,omitempty"` + ModelID string `json:"model,omitempty"` +} + +type BrickConfigVariable struct { + Name string `json:"name"` + Value string `json:"value"` + Description string `json:"description"` + Required bool `json:"required"` } type BrickVariable struct {