Skip to content

Commit c64fbf6

Browse files
mirkoCrobumirkoCrobu
authored
feat(api): return variable configurations in /apps/:id/bricks and apps/:id/bricks/:id (#18)
* add variables_details * add unit test * update openapi * add test e2e * fix bug * add deprecated field description for openapi * refactoring test constants * restore license header * rename variables_details to config_variables * rename variable name * udate docs and tests e2e * refactoring variables
1 parent cd53bab commit c64fbf6

File tree

8 files changed

+205
-43
lines changed

8 files changed

+205
-43
lines changed

‎internal/api/docs/openapi.yaml‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1276,6 +1276,17 @@ components:
12761276
name:
12771277
type: string
12781278
type: object
1279+
BrickConfigVariable:
1280+
properties:
1281+
description:
1282+
type: string
1283+
name:
1284+
type: string
1285+
required:
1286+
type: boolean
1287+
value:
1288+
type: string
1289+
type: object
12791290
BrickCreateUpdateRequest:
12801291
properties:
12811292
model:
@@ -1325,6 +1336,10 @@ components:
13251336
type: string
13261337
category:
13271338
type: string
1339+
config_variables:
1340+
items:
1341+
$ref: '#/components/schemas/BrickConfigVariable'
1342+
type: array
13281343
id:
13291344
type: string
13301345
model:
@@ -1336,6 +1351,8 @@ components:
13361351
variables:
13371352
additionalProperties:
13381353
type: string
1354+
description: 'Deprecated: use config_variables instead. This field is kept
1355+
for backward compatibility.'
13391356
type: object
13401357
type: object
13411358
BrickListItem:

‎internal/e2e/client/client.gen.go‎

Lines changed: 17 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/e2e/daemon/app_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,7 @@ func TestDeleteApp(t *testing.T) {
470470

471471
t.Run("DeletingExampleApp_Fail", func(t *testing.T) {
472472
var actualResponseBody models.ErrorResponse
473-
deleteResp, err := httpClient.DeleteApp(t.Context(), noExisitingExample)
473+
deleteResp, err := httpClient.DeleteApp(t.Context(), "ZXhhbXBsZXM6anVzdGJsaW5f")
474474
require.NoError(t, err)
475475
defer deleteResp.Body.Close()
476476

@@ -818,7 +818,7 @@ func TestAppPorts(t *testing.T) {
818818
respBrick, err := httpClient.UpsertAppBrickInstanceWithResponse(
819819
t.Context(),
820820
*createResp.JSON201.Id,
821-
StreamLitUi,
821+
"arduino:streamlit_ui",
822822
client.BrickCreateUpdateRequest{},
823823
func(ctx context.Context, req *http.Request) error { return nil },
824824
)

‎internal/e2e/daemon/const.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,7 @@
1616
package daemon
1717

1818
const (
19-
ImageClassifactionBrickID = "arduino:image_classification"
20-
StreamLitUi = "arduino:streamlit_ui"
21-
expectedDetailsAppNotfound = "unable to find the app"
22-
expectedDetailsAppInvalidAppId = "invalid app id"
23-
noExistingApp = "dXNlcjp0ZXN0LWFwcAw"
24-
malformedAppId = "this-is-definitely-not-base64"
25-
noExisitingExample = "ZXhhbXBsZXM6anVzdGJsaW5f"
19+
ImageClassifactionBrickID = "arduino:image_classification"
20+
noExistingApp = "dXNlcjp0ZXN0LWFwcAw"
21+
malformedAppId = "this-is-definitely-not-base64"
2622
)

‎internal/e2e/daemon/instance_bricks_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,28 @@ import (
3131
"github.com/arduino/arduino-app-cli/internal/e2e/client"
3232
)
3333

34+
const (
35+
expectedDetailsAppInvalidAppId = "invalid app id"
36+
expectedDetailsAppNotfound = "unable to find the app"
37+
)
38+
39+
var (
40+
expectedConfigVariables = []client.BrickConfigVariable{
41+
{
42+
Description: f.Ptr("path to the custom model directory"),
43+
Name: f.Ptr("CUSTOM_MODEL_PATH"),
44+
Required: f.Ptr(false),
45+
Value: f.Ptr("/home/arduino/.arduino-bricks/ei-models"),
46+
},
47+
{
48+
Description: f.Ptr("path to the model file"),
49+
Name: f.Ptr("EI_CLASSIFICATION_MODEL"),
50+
Required: f.Ptr(false),
51+
Value: f.Ptr("/models/ootb/ei/mobilenet-v2-224px.eim"),
52+
},
53+
}
54+
)
55+
3456
func setupTestApp(t *testing.T) (*client.CreateAppResp, *client.ClientWithResponses) {
3557
httpClient := GetHttpclient(t)
3658
createResp, err := httpClient.CreateAppWithResponse(
@@ -68,6 +90,7 @@ func TestGetAppBrickInstances(t *testing.T) {
6890
require.NoError(t, err)
6991
require.Len(t, *brickInstances.JSON200.Bricks, 1)
7092
require.Equal(t, ImageClassifactionBrickID, *(*brickInstances.JSON200.Bricks)[0].Id)
93+
require.Equal(t, expectedConfigVariables, *(*brickInstances.JSON200.Bricks)[0].ConfigVariables)
7194

7295
})
7396

@@ -111,6 +134,7 @@ func TestGetAppBrickInstanceById(t *testing.T) {
111134
require.NoError(t, err)
112135
require.NotEmpty(t, brickInstance.JSON200)
113136
require.Equal(t, ImageClassifactionBrickID, *brickInstance.JSON200.Id)
137+
require.Equal(t, expectedConfigVariables, (*brickInstance.JSON200.ConfigVariables))
114138
})
115139

116140
t.Run("GetAppBrickInstanceByBrickID_InvalidAppID_Fails", func(t *testing.T) {

‎internal/orchestrator/bricks/bricks.go‎

Lines changed: 47 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
"errors"
2020
"fmt"
2121
"log/slog"
22-
"maps"
2322
"slices"
2423

2524
"github.com/arduino/go-paths-helper"
@@ -80,15 +79,20 @@ func (s *Service) AppBrickInstancesList(a *app.ArduinoApp) (AppBrickInstancesRes
8079
if !found {
8180
return AppBrickInstancesResult{}, fmt.Errorf("brick not found with id %s", brickInstance.ID)
8281
}
82+
83+
variablesMap, configVariables := getBrickConfigDetails(brick, brickInstance.Variables)
84+
8385
res.BrickInstances[i] = BrickInstance{
84-
ID: brick.ID,
85-
Name: brick.Name,
86-
Author: "Arduino", // TODO: for now we only support our bricks
87-
Category: brick.Category,
88-
Status: "installed",
89-
ModelID: brickInstance.Model, // TODO: in case is not set by the user, should we return the default model?
90-
Variables: brickInstance.Variables, // TODO: do we want to show also the default value of not explicitly set variables?
86+
ID: brick.ID,
87+
Name: brick.Name,
88+
Author: "Arduino", // TODO: for now we only support our bricks
89+
Category: brick.Category,
90+
Status: "installed",
91+
ModelID: brickInstance.Model, // TODO: in case is not set by the user, should we return the default model?
92+
Variables: variablesMap, // TODO: do we want to show also the default value of not explicitly set variables?
93+
ConfigVariables: configVariables,
9194
}
95+
9296
}
9397
return res, nil
9498
}
@@ -104,29 +108,51 @@ func (s *Service) AppBrickInstanceDetails(a *app.ArduinoApp, brickID string) (Br
104108
return BrickInstance{}, fmt.Errorf("brick %s not added in the app", brickID)
105109
}
106110

107-
variables := make(map[string]string, len(brick.Variables))
108-
for _, v := range brick.Variables {
109-
variables[v.Name] = v.DefaultValue
110-
}
111-
// Add/Update the variables with the ones from the app descriptor
112-
maps.Copy(variables, a.Descriptor.Bricks[brickIndex].Variables)
111+
variables, configVariables := getBrickConfigDetails(brick, a.Descriptor.Bricks[brickIndex].Variables)
113112

114113
modelID := a.Descriptor.Bricks[brickIndex].Model
115114
if modelID == "" {
116115
modelID = brick.ModelName
117116
}
118117

119118
return BrickInstance{
120-
ID: brickID,
121-
Name: brick.Name,
122-
Author: "Arduino", // TODO: for now we only support our bricks
123-
Category: brick.Category,
124-
Status: "installed", // For now every Arduino brick are installed
125-
Variables: variables,
126-
ModelID: modelID,
119+
ID: brickID,
120+
Name: brick.Name,
121+
Author: "Arduino", // TODO: for now we only support our bricks
122+
Category: brick.Category,
123+
Status: "installed", // For now every Arduino brick are installed
124+
Variables: variables,
125+
ConfigVariables: configVariables,
126+
ModelID: modelID,
127127
}, nil
128128
}
129129

130+
func getBrickConfigDetails(
131+
brick *bricksindex.Brick, userVariables map[string]string,
132+
) (map[string]string, []BrickConfigVariable) {
133+
variablesMap := make(map[string]string, len(brick.Variables))
134+
variableDetails := make([]BrickConfigVariable, 0, len(brick.Variables))
135+
136+
for _, v := range brick.Variables {
137+
finalValue := v.DefaultValue
138+
139+
userValue, ok := userVariables[v.Name]
140+
if ok {
141+
finalValue = userValue
142+
}
143+
variablesMap[v.Name] = finalValue
144+
145+
variableDetails = append(variableDetails, BrickConfigVariable{
146+
Name: v.Name,
147+
Value: finalValue,
148+
Description: v.Description,
149+
Required: v.IsRequired(),
150+
})
151+
}
152+
153+
return variablesMap, variableDetails
154+
}
155+
130156
func (s *Service) BricksDetails(id string, idProvider *app.IDProvider,
131157
cfg config.Configuration) (BrickDetailsResult, error) {
132158
brick, found := s.bricksIndex.FindBrickByID(id)

‎internal/orchestrator/bricks/bricks_test.go‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,3 +110,83 @@ func TestBrickCreate(t *testing.T) {
110110
require.Equal(t, secret, after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"])
111111
})
112112
}
113+
114+
func TestGetBrickInstanceVariableDetails(t *testing.T) {
115+
tests := []struct {
116+
name string
117+
brick *bricksindex.Brick
118+
userVariables map[string]string
119+
expectedConfigVariables []BrickConfigVariable
120+
expectedVariableMap map[string]string
121+
}{
122+
{
123+
name: "variable is present in the map",
124+
brick: &bricksindex.Brick{
125+
Variables: []bricksindex.BrickVariable{
126+
{Name: "VAR1", Description: "desc"},
127+
},
128+
},
129+
userVariables: map[string]string{"VAR1": "value1"},
130+
expectedConfigVariables: []BrickConfigVariable{
131+
{Name: "VAR1", Value: "value1", Description: "desc", Required: true},
132+
},
133+
expectedVariableMap: map[string]string{"VAR1": "value1"},
134+
},
135+
{
136+
name: "variable not present in the map",
137+
brick: &bricksindex.Brick{
138+
Variables: []bricksindex.BrickVariable{
139+
{Name: "VAR1", Description: "desc"},
140+
},
141+
},
142+
userVariables: map[string]string{},
143+
expectedConfigVariables: []BrickConfigVariable{
144+
{Name: "VAR1", Value: "", Description: "desc", Required: true},
145+
},
146+
expectedVariableMap: map[string]string{"VAR1": ""},
147+
},
148+
{
149+
name: "variable with default value",
150+
brick: &bricksindex.Brick{
151+
Variables: []bricksindex.BrickVariable{
152+
{Name: "VAR1", DefaultValue: "default", Description: "desc"},
153+
},
154+
},
155+
userVariables: map[string]string{},
156+
expectedConfigVariables: []BrickConfigVariable{
157+
{Name: "VAR1", Value: "default", Description: "desc", Required: false},
158+
},
159+
expectedVariableMap: map[string]string{"VAR1": "default"},
160+
},
161+
{
162+
name: "multiple variables",
163+
brick: &bricksindex.Brick{
164+
Variables: []bricksindex.BrickVariable{
165+
{Name: "VAR1", Description: "desc1"},
166+
{Name: "VAR2", DefaultValue: "def2", Description: "desc2"},
167+
},
168+
},
169+
userVariables: map[string]string{"VAR1": "v1"},
170+
expectedConfigVariables: []BrickConfigVariable{
171+
{Name: "VAR1", Value: "v1", Description: "desc1", Required: true},
172+
{Name: "VAR2", Value: "def2", Description: "desc2", Required: false},
173+
},
174+
expectedVariableMap: map[string]string{"VAR1": "v1", "VAR2": "def2"},
175+
},
176+
{
177+
name: "no variables",
178+
brick: &bricksindex.Brick{Variables: []bricksindex.BrickVariable{}},
179+
userVariables: map[string]string{},
180+
expectedConfigVariables: []BrickConfigVariable{},
181+
expectedVariableMap: map[string]string{},
182+
},
183+
}
184+
185+
for _, tt := range tests {
186+
t.Run(tt.name, func(t *testing.T) {
187+
actualVariableMap, actualConfigVariables := getBrickConfigDetails(tt.brick, tt.userVariables)
188+
require.Equal(t, tt.expectedVariableMap, actualVariableMap)
189+
require.Equal(t, tt.expectedConfigVariables, actualConfigVariables)
190+
})
191+
}
192+
}

‎internal/orchestrator/bricks/types.go‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,21 @@ type AppBrickInstancesResult struct {
3434
}
3535

3636
type BrickInstance struct {
37-
ID string `json:"id"`
38-
Name string `json:"name"`
39-
Author string `json:"author"`
40-
Category string `json:"category"`
41-
Status string `json:"status"`
42-
Variables map[string]string `json:"variables,omitempty"`
43-
ModelID string `json:"model,omitempty"`
37+
ID string `json:"id"`
38+
Name string `json:"name"`
39+
Author string `json:"author"`
40+
Category string `json:"category"`
41+
Status string `json:"status"`
42+
Variables map[string]string `json:"variables,omitempty" description:"Deprecated: use config_variables instead. This field is kept for backward compatibility."`
43+
ConfigVariables []BrickConfigVariable `json:"config_variables,omitempty"`
44+
ModelID string `json:"model,omitempty"`
45+
}
46+
47+
type BrickConfigVariable struct {
48+
Name string `json:"name"`
49+
Value string `json:"value"`
50+
Description string `json:"description"`
51+
Required bool `json:"required"`
4452
}
4553

4654
type BrickVariable struct {

0 commit comments

Comments
 (0)