Skip to content

Commit 18b6daf

Browse files
dido18dido18
committed
refactor: improve error messages and update brick handling in service methods
1 parent 98a2ae1 commit 18b6daf

File tree

4 files changed

+81
-82
lines changed

4 files changed

+81
-82
lines changed

‎internal/api/handlers/bricks.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ func HandleBrickUpdates(
213213
req.ID = id
214214
err = brickService.BrickUpdate(req, app)
215215
if err != nil {
216-
slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()))
216+
slog.Error("Unable to update the brick", slog.String("error", err.Error()))
217217
render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to update the brick"})
218218

219219
return

‎internal/orchestrator/app/validator.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func ValidateBricks(a AppDescriptor, index *bricksindex.BricksIndex) error {
2828
_, exist := indexBrick.GetVariable(appBrickVariableName)
2929
if !exist {
3030
// TODO: we should return warnings
31-
slog.Warn("variable is referenced but not declared in the brick configuration", "variable", appBrickVariableName, "brick", indexBrick.ID)
31+
slog.Warn("[skip] variable does not exist into the brick definition", "variable", appBrickVariableName, "brick", indexBrick.ID)
3232
}
3333
}
3434

‎internal/orchestrator/bricks/bricks.go‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -335,20 +335,21 @@ func (s *Service) BrickUpdate(
335335
req BrickCreateUpdateRequest,
336336
appCurrent app.ArduinoApp,
337337
) error {
338-
_, present := s.bricksIndex.FindBrickByID(req.ID)
338+
brickFromIndex, present := s.bricksIndex.FindBrickByID(req.ID)
339339
if !present {
340340
return fmt.Errorf("brick %q not found into the brick index", req.ID)
341341
}
342-
index := slices.IndexFunc(appCurrent.Descriptor.Bricks, func(b app.Brick) bool { return b.ID == req.ID })
343-
if index == -1 {
342+
343+
brickPosition := slices.IndexFunc(appCurrent.Descriptor.Bricks, func(b app.Brick) bool { return b.ID == req.ID })
344+
if brickPosition == -1 {
344345
return fmt.Errorf("brick %q not found into the bricks of the app", req.ID)
345346
}
346-
brickID := appCurrent.Descriptor.Bricks[index].ID
347-
brickVariables := appCurrent.Descriptor.Bricks[index].Variables
347+
348+
brickVariables := appCurrent.Descriptor.Bricks[brickPosition].Variables
348349
if len(brickVariables) == 0 {
349350
brickVariables = make(map[string]string)
350351
}
351-
brickModel := appCurrent.Descriptor.Bricks[index].Model
352+
brickModel := appCurrent.Descriptor.Bricks[brickPosition].Model
352353

353354
if req.Model != nil && *req.Model != brickModel {
354355
models := s.modelsIndex.GetModelsByBrick(req.ID)
@@ -358,15 +359,11 @@ func (s *Service) BrickUpdate(
358359
}
359360
brickModel = *req.Model
360361
}
361-
brick, present := s.bricksIndex.FindBrickByID(brickID)
362-
if !present {
363-
return fmt.Errorf("brick %q not found in the brick index", brickID)
364-
}
365362

366363
for name, updateValue := range req.Variables {
367-
value, exist := brick.GetVariable(name)
364+
value, exist := brickFromIndex.GetVariable(name)
368365
if !exist {
369-
return fmt.Errorf("variable %q does not exist on brick %q", name, brick.ID)
366+
return fmt.Errorf("variable %q does not exist on brick %q", name, brickFromIndex.ID)
370367
}
371368
if value.IsRequired() && updateValue == "" {
372369
return fmt.Errorf("required variable %q cannot be empty", name)
@@ -384,16 +381,16 @@ func (s *Service) BrickUpdate(
384381
}
385382
}
386383

387-
for _, brickVar := range brick.Variables {
384+
for _, brickVar := range brickFromIndex.Variables {
388385
if brickVar.IsRequired() {
389386
if _, exist := req.Variables[brickVar.Name]; !exist {
390387
return fmt.Errorf("required variable %q must be set", brickVar.Name)
391388
}
392389
}
393390
}
394391

395-
appCurrent.Descriptor.Bricks[index].Model = brickModel
396-
appCurrent.Descriptor.Bricks[index].Variables = brickVariables
392+
appCurrent.Descriptor.Bricks[brickPosition].Model = brickModel
393+
appCurrent.Descriptor.Bricks[brickPosition].Variables = brickVariables
397394

398395
err := appCurrent.Save()
399396
if err != nil {

‎internal/orchestrator/bricks/bricks_test.go‎

Lines changed: 67 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -26,28 +26,22 @@ import (
2626
"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
2727
)
2828

29-
func TestUdateBrick(t *testing.T) {
29+
func TestBrickCreate(t *testing.T) {
3030
bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
3131
require.Nil(t, err)
3232
brickService := NewService(nil, bricksIndex, nil)
3333

34-
t.Run("fails if brick id does not exist into brick index", func(t *testing.T) {
35-
err = brickService.BrickUpdate(BrickCreateUpdateRequest{ID: "not-existing-id"}, f.Must(app.Load("testdata/dummy-app")))
36-
require.Error(t, err)
37-
require.Equal(t, "brick \"not-existing-id\" not found into the brick index", err.Error())
38-
})
39-
40-
t.Run("fails if brick id is present into the index but not in the app ", func(t *testing.T) {
41-
err = brickService.BrickUpdate(BrickCreateUpdateRequest{ID: "arduino:dbstorage_sqlstore"}, f.Must(app.Load("testdata/dummy-app")))
34+
t.Run("fails if brick id does not exist", func(t *testing.T) {
35+
err = brickService.BrickCreate(BrickCreateUpdateRequest{ID: "not-existing-id"}, f.Must(app.Load("testdata/dummy-app")))
4236
require.Error(t, err)
43-
require.Equal(t, "brick \"not-existing-id\" not found into app descriptor", err.Error())
37+
require.Equal(t, "brick \"not-existing-id\" not found", err.Error())
4438
})
4539

46-
t.Run("fails if the updated variable is not present in the brick definition", func(t *testing.T) {
40+
t.Run("fails if the requestes variable is not present in the brick definition", func(t *testing.T) {
4741
req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{
4842
"NON_EXISTING_VARIABLE": "some-value",
4943
}}
50-
err = brickService.BrickUpdate(req, f.Must(app.Load("testdata/dummy-app")))
44+
err = brickService.BrickCreate(req, f.Must(app.Load("testdata/dummy-app")))
5145
require.Error(t, err)
5246
require.Equal(t, "variable \"NON_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\"", err.Error())
5347
})
@@ -57,12 +51,12 @@ func TestUdateBrick(t *testing.T) {
5751
"ARDUINO_DEVICE_ID": "",
5852
"ARDUINO_SECRET": "a-secret-a",
5953
}}
60-
err = brickService.BrickUpdate(req, f.Must(app.Load("testdata/dummy-app")))
54+
err = brickService.BrickCreate(req, f.Must(app.Load("testdata/dummy-app")))
6155
require.Error(t, err)
6256
require.Equal(t, "required variable \"ARDUINO_DEVICE_ID\" cannot be empty", err.Error())
6357
})
6458

65-
t.Run("fails if a mandatory variable is not present", func(t *testing.T) {
59+
t.Run("do not fail if a mandatory variable is not present", func(t *testing.T) {
6660
tempDummyApp := paths.New("testdata/dummy-app.temp")
6761
err := tempDummyApp.RemoveAll()
6862
require.Nil(t, err)
@@ -71,21 +65,44 @@ func TestUdateBrick(t *testing.T) {
7165
req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{
7266
"ARDUINO_SECRET": "a-secret-a",
7367
}}
74-
err = brickService.BrickUpdate(req, f.Must(app.Load(tempDummyApp.String())))
75-
require.Error(t, err)
76-
require.Equal(t, "required variable \"ARDUINO_DEVICE_ID\" must be set", err.Error())
68+
err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String())))
69+
require.NoError(t, err)
70+
71+
after, err := app.Load(tempDummyApp.String())
72+
require.Nil(t, err)
73+
require.Len(t, after.Descriptor.Bricks, 1)
74+
require.Equal(t, "arduino:arduino_cloud", after.Descriptor.Bricks[0].ID)
75+
require.Equal(t, "", after.Descriptor.Bricks[0].Variables["ARDUINO_DEVICE_ID"])
76+
require.Equal(t, "a-secret-a", after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"])
7777
})
7878

79-
t.Run("update the variables of a brick correctly", func(t *testing.T) {
80-
tempDummyApp := paths.New("testdata/dummy-app.brick-override.temp")
81-
require.Nil(t, tempDummyApp.RemoveAll())
79+
t.Run("the brick is added if it does not exist in the app", func(t *testing.T) {
80+
tempDummyApp := paths.New("testdata/dummy-app.temp")
81+
err := tempDummyApp.RemoveAll()
82+
require.Nil(t, err)
8283
require.Nil(t, paths.New("testdata/dummy-app").CopyDirTo(tempDummyApp))
84+
85+
req := BrickCreateUpdateRequest{ID: "arduino:dbstorage_sqlstore"}
86+
err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String())))
87+
require.Nil(t, err)
88+
after, err := app.Load(tempDummyApp.String())
89+
require.Nil(t, err)
90+
require.Len(t, after.Descriptor.Bricks, 2)
91+
require.Equal(t, "arduino:dbstorage_sqlstore", after.Descriptor.Bricks[1].ID)
92+
})
93+
94+
t.Run("the variables of a brick are updated", func(t *testing.T) {
95+
tempDummyApp := paths.New("testdata/dummy-app.brick-override.temp")
96+
err := tempDummyApp.RemoveAll()
97+
require.Nil(t, err)
98+
err = paths.New("testdata/dummy-app").CopyDirTo(tempDummyApp)
99+
require.Nil(t, err)
83100
bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
84101
require.Nil(t, err)
85102
brickService := NewService(nil, bricksIndex, nil)
86103

87-
deviceID := "updated-device-id"
88-
secret := "updated-secret"
104+
deviceID := "this-is-a-device-id"
105+
secret := "this-is-a-secret"
89106
req := BrickCreateUpdateRequest{
90107
ID: "arduino:arduino_cloud",
91108
Variables: map[string]string{
@@ -94,7 +111,7 @@ func TestUdateBrick(t *testing.T) {
94111
},
95112
}
96113

97-
err = brickService.BrickUpdate(req, f.Must(app.Load(tempDummyApp.String())))
114+
err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String())))
98115
require.Nil(t, err)
99116

100117
after, err := app.Load(tempDummyApp.String())
@@ -104,24 +121,31 @@ func TestUdateBrick(t *testing.T) {
104121
require.Equal(t, deviceID, after.Descriptor.Bricks[0].Variables["ARDUINO_DEVICE_ID"])
105122
require.Equal(t, secret, after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"])
106123
})
107-
108124
}
109-
func TestBrickCreate(t *testing.T) {
125+
126+
func TestUdateBrick(t *testing.T) {
110127
bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
111128
require.Nil(t, err)
112129
brickService := NewService(nil, bricksIndex, nil)
113130

114-
t.Run("fails if brick id does not exist", func(t *testing.T) {
115-
err = brickService.BrickCreate(BrickCreateUpdateRequest{ID: "not-existing-id"}, f.Must(app.Load("testdata/dummy-app")))
131+
t.Run("fails if brick id does not exist into brick index", func(t *testing.T) {
132+
err = brickService.BrickUpdate(BrickCreateUpdateRequest{ID: "not-existing-id"}, f.Must(app.Load("testdata/dummy-app")))
116133
require.Error(t, err)
117-
require.Equal(t, "brick \"not-existing-id\" not found", err.Error())
134+
require.Equal(t, "brick \"not-existing-id\" not found into the brick index", err.Error())
118135
})
119136

120-
t.Run("fails if the requestes variable is not present in the brick definition", func(t *testing.T) {
137+
// TODO: if a brick is updated but it is corrunlty not in the app.yaml ? ithink we must add it to the app.yaml
138+
t.Run("fails if brick is present into the index but not in the app ", func(t *testing.T) {
139+
err = brickService.BrickUpdate(BrickCreateUpdateRequest{ID: "arduino:dbstorage_sqlstore"}, f.Must(app.Load("testdata/dummy-app")))
140+
require.Error(t, err)
141+
require.Equal(t, "brick \"arduino:dbstorage_sqlstore\" not found into the bricks of the app", err.Error())
142+
})
143+
144+
t.Run("fails if the updated variable is not present in the brick definition", func(t *testing.T) {
121145
req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{
122146
"NON_EXISTING_VARIABLE": "some-value",
123147
}}
124-
err = brickService.BrickCreate(req, f.Must(app.Load("testdata/dummy-app")))
148+
err = brickService.BrickUpdate(req, f.Must(app.Load("testdata/dummy-app")))
125149
require.Error(t, err)
126150
require.Equal(t, "variable \"NON_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\"", err.Error())
127151
})
@@ -131,12 +155,12 @@ func TestBrickCreate(t *testing.T) {
131155
"ARDUINO_DEVICE_ID": "",
132156
"ARDUINO_SECRET": "a-secret-a",
133157
}}
134-
err = brickService.BrickCreate(req, f.Must(app.Load("testdata/dummy-app")))
158+
err = brickService.BrickUpdate(req, f.Must(app.Load("testdata/dummy-app")))
135159
require.Error(t, err)
136-
require.Equal(t, "variable \"ARDUINO_DEVICE_ID\" cannot be empty", err.Error())
160+
require.Equal(t, "required variable \"ARDUINO_DEVICE_ID\" cannot be empty", err.Error())
137161
})
138162

139-
t.Run("do not fail if a mandatory variable is not present", func(t *testing.T) {
163+
t.Run("fails if a mandatory variable is not present", func(t *testing.T) {
140164
tempDummyApp := paths.New("testdata/dummy-app.temp")
141165
err := tempDummyApp.RemoveAll()
142166
require.Nil(t, err)
@@ -145,44 +169,21 @@ func TestBrickCreate(t *testing.T) {
145169
req := BrickCreateUpdateRequest{ID: "arduino:arduino_cloud", Variables: map[string]string{
146170
"ARDUINO_SECRET": "a-secret-a",
147171
}}
148-
err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String())))
149-
require.NoError(t, err)
150-
151-
after, err := app.Load(tempDummyApp.String())
152-
require.Nil(t, err)
153-
require.Len(t, after.Descriptor.Bricks, 1)
154-
require.Equal(t, "arduino:arduino_cloud", after.Descriptor.Bricks[0].ID)
155-
require.Equal(t, "", after.Descriptor.Bricks[0].Variables["ARDUINO_DEVICE_ID"])
156-
require.Equal(t, "a-secret-a", after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"])
157-
})
158-
159-
t.Run("the brick is added if it does not exist in the app", func(t *testing.T) {
160-
tempDummyApp := paths.New("testdata/dummy-app.temp")
161-
err := tempDummyApp.RemoveAll()
162-
require.Nil(t, err)
163-
require.Nil(t, paths.New("testdata/dummy-app").CopyDirTo(tempDummyApp))
164-
165-
req := BrickCreateUpdateRequest{ID: "arduino:dbstorage_sqlstore"}
166-
err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String())))
167-
require.Nil(t, err)
168-
after, err := app.Load(tempDummyApp.String())
169-
require.Nil(t, err)
170-
require.Len(t, after.Descriptor.Bricks, 2)
171-
require.Equal(t, "arduino:dbstorage_sqlstore", after.Descriptor.Bricks[1].ID)
172+
err = brickService.BrickUpdate(req, f.Must(app.Load(tempDummyApp.String())))
173+
require.Error(t, err)
174+
require.Equal(t, "required variable \"ARDUINO_DEVICE_ID\" must be set", err.Error())
172175
})
173176

174-
t.Run("the variables of a brick are updated", func(t *testing.T) {
177+
t.Run("update the variables of a brick correctly", func(t *testing.T) {
175178
tempDummyApp := paths.New("testdata/dummy-app.brick-override.temp")
176-
err := tempDummyApp.RemoveAll()
177-
require.Nil(t, err)
178-
err = paths.New("testdata/dummy-app").CopyDirTo(tempDummyApp)
179-
require.Nil(t, err)
179+
require.Nil(t, tempDummyApp.RemoveAll())
180+
require.Nil(t, paths.New("testdata/dummy-app").CopyDirTo(tempDummyApp))
180181
bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata"))
181182
require.Nil(t, err)
182183
brickService := NewService(nil, bricksIndex, nil)
183184

184-
deviceID := "this-is-a-device-id"
185-
secret := "this-is-a-secret"
185+
deviceID := "updated-device-id"
186+
secret := "updated-secret"
186187
req := BrickCreateUpdateRequest{
187188
ID: "arduino:arduino_cloud",
188189
Variables: map[string]string{
@@ -191,7 +192,7 @@ func TestBrickCreate(t *testing.T) {
191192
},
192193
}
193194

194-
err = brickService.BrickCreate(req, f.Must(app.Load(tempDummyApp.String())))
195+
err = brickService.BrickUpdate(req, f.Must(app.Load(tempDummyApp.String())))
195196
require.Nil(t, err)
196197

197198
after, err := app.Load(tempDummyApp.String())
@@ -201,6 +202,7 @@ func TestBrickCreate(t *testing.T) {
201202
require.Equal(t, deviceID, after.Descriptor.Bricks[0].Variables["ARDUINO_DEVICE_ID"])
202203
require.Equal(t, secret, after.Descriptor.Bricks[0].Variables["ARDUINO_SECRET"])
203204
})
205+
204206
}
205207

206208
func TestGetBrickInstanceVariableDetails(t *testing.T) {

0 commit comments

Comments
 (0)