Skip to content

Commit 9525e01

Browse files
gabriel-samfiragabriel-samfira
authored
Merge pull request #241 from gabriel-samfira/some-cleanup
Slightly simplify code
2 parents 9cbb2f8 + 36288c6 commit 9525e01

File tree

7 files changed

+42
-146
lines changed

7 files changed

+42
-146
lines changed

‎database/common/common.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ type UserStore interface {
7474
type InstanceStore interface {
7575
CreateInstance(ctx context.Context, poolID string, param params.CreateInstanceParams) (params.Instance, error)
7676
DeleteInstance(ctx context.Context, poolID string, instanceName string) error
77-
UpdateInstance(ctx context.Context, instanceID string, param params.UpdateInstanceParams) (params.Instance, error)
77+
UpdateInstance(ctx context.Context, instanceName string, param params.UpdateInstanceParams) (params.Instance, error)
7878

7979
// Probably a bad idea without some king of filter or at least pagination
8080
//
@@ -83,8 +83,7 @@ type InstanceStore interface {
8383
ListAllInstances(ctx context.Context) ([]params.Instance, error)
8484

8585
GetInstanceByName(ctx context.Context, instanceName string) (params.Instance, error)
86-
AddInstanceEvent(ctx context.Context, instanceID string, event params.EventType, eventLevel params.EventLevel, eventMessage string) error
87-
ListInstanceEvents(ctx context.Context, instanceID string, eventType params.EventType, eventLevel params.EventLevel) ([]params.StatusMessage, error)
86+
AddInstanceEvent(ctx context.Context, instanceName string, event params.EventType, eventLevel params.EventLevel, eventMessage string) error
8887
}
8988

9089
type JobsStore interface {

‎database/common/mocks/Store.go‎

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

‎database/sql/instances.go‎

Lines changed: 4 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -92,22 +92,6 @@ func (s *sqlDatabase) CreateInstance(_ context.Context, poolID string, param par
9292
return s.sqlToParamsInstance(newInstance)
9393
}
9494

95-
func (s *sqlDatabase) getInstanceByID(_ context.Context, instanceID string) (Instance, error) {
96-
u, err := uuid.Parse(instanceID)
97-
if err != nil {
98-
return Instance{}, errors.Wrap(runnerErrors.ErrBadRequest, "parsing id")
99-
}
100-
var instance Instance
101-
q := s.conn.Model(&Instance{}).
102-
Preload(clause.Associations).
103-
Where("id = ?", u).
104-
First(&instance)
105-
if q.Error != nil {
106-
return Instance{}, errors.Wrap(q.Error, "fetching instance")
107-
}
108-
return instance, nil
109-
}
110-
11195
func (s *sqlDatabase) getPoolInstanceByName(poolID string, instanceName string) (Instance, error) {
11296
pool, err := s.getPoolByID(s.conn, poolID)
11397
if err != nil {
@@ -184,34 +168,8 @@ func (s *sqlDatabase) DeleteInstance(_ context.Context, poolID string, instanceN
184168
return nil
185169
}
186170

187-
func (s *sqlDatabase) ListInstanceEvents(_ context.Context, instanceID string, eventType params.EventType, eventLevel params.EventLevel) ([]params.StatusMessage, error) {
188-
var events []InstanceStatusUpdate
189-
query := s.conn.Model(&InstanceStatusUpdate{}).Where("instance_id = ?", instanceID)
190-
if eventLevel != "" {
191-
query = query.Where("event_level = ?", eventLevel)
192-
}
193-
194-
if eventType != "" {
195-
query = query.Where("event_type = ?", eventType)
196-
}
197-
198-
if result := query.Find(&events); result.Error != nil {
199-
return nil, errors.Wrap(result.Error, "fetching events")
200-
}
201-
202-
eventParams := make([]params.StatusMessage, len(events))
203-
for idx, val := range events {
204-
eventParams[idx] = params.StatusMessage{
205-
Message: val.Message,
206-
EventType: val.EventType,
207-
EventLevel: val.EventLevel,
208-
}
209-
}
210-
return eventParams, nil
211-
}
212-
213-
func (s *sqlDatabase) AddInstanceEvent(ctx context.Context, instanceID string, event params.EventType, eventLevel params.EventLevel, statusMessage string) error {
214-
instance, err := s.getInstanceByID(ctx, instanceID)
171+
func (s *sqlDatabase) AddInstanceEvent(ctx context.Context, instanceName string, event params.EventType, eventLevel params.EventLevel, statusMessage string) error {
172+
instance, err := s.getInstanceByName(ctx, instanceName)
215173
if err != nil {
216174
return errors.Wrap(err, "updating instance")
217175
}
@@ -228,8 +186,8 @@ func (s *sqlDatabase) AddInstanceEvent(ctx context.Context, instanceID string, e
228186
return nil
229187
}
230188

231-
func (s *sqlDatabase) UpdateInstance(ctx context.Context, instanceID string, param params.UpdateInstanceParams) (params.Instance, error) {
232-
instance, err := s.getInstanceByID(ctx, instanceID)
189+
func (s *sqlDatabase) UpdateInstance(ctx context.Context, instanceName string, param params.UpdateInstanceParams) (params.Instance, error) {
190+
instance, err := s.getInstanceByName(ctx, instanceName)
233191
if err != nil {
234192
return params.Instance{}, errors.Wrap(err, "updating instance")
235193
}

‎database/sql/instances_test.go‎

Lines changed: 14 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,7 @@ func (s *InstancesTestSuite) TestAddInstanceEvent() {
357357
storeInstance := s.Fixtures.Instances[0]
358358
statusMsg := "test-status-message"
359359

360-
err := s.Store.AddInstanceEvent(context.Background(), storeInstance.ID, params.StatusEvent, params.EventInfo, statusMsg)
360+
err := s.Store.AddInstanceEvent(context.Background(), storeInstance.Name, params.StatusEvent, params.EventInfo, statusMsg)
361361

362362
s.Require().Nil(err)
363363
instance, err := s.Store.GetInstanceByName(context.Background(), storeInstance.Name)
@@ -368,19 +368,13 @@ func (s *InstancesTestSuite) TestAddInstanceEvent() {
368368
s.Require().Equal(statusMsg, instance.StatusMessages[0].Message)
369369
}
370370

371-
func (s *InstancesTestSuite) TestAddInstanceEventInvalidPoolID() {
372-
err := s.Store.AddInstanceEvent(context.Background(), "dummy-id", params.StatusEvent, params.EventInfo, "dummy-message")
373-
374-
s.Require().Equal("updating instance: parsing id: invalid request", err.Error())
375-
}
376-
377371
func (s *InstancesTestSuite) TestAddInstanceEventDBUpdateErr() {
378372
instance := s.Fixtures.Instances[0]
379373
statusMsg := "test-status-message"
380374

381375
s.Fixtures.SQLMock.
382-
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE id = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")).
383-
WithArgs(instance.ID).
376+
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE name = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")).
377+
WithArgs(instance.Name).
384378
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(instance.ID))
385379
s.Fixtures.SQLMock.
386380
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `addresses` WHERE `addresses`.`instance_id` = ? AND `addresses`.`deleted_at` IS NULL")).
@@ -404,15 +398,15 @@ func (s *InstancesTestSuite) TestAddInstanceEventDBUpdateErr() {
404398
WillReturnError(fmt.Errorf("mocked add status message error"))
405399
s.Fixtures.SQLMock.ExpectRollback()
406400

407-
err := s.StoreSQLMocked.AddInstanceEvent(context.Background(), instance.ID, params.StatusEvent, params.EventInfo, statusMsg)
401+
err := s.StoreSQLMocked.AddInstanceEvent(context.Background(), instance.Name, params.StatusEvent, params.EventInfo, statusMsg)
408402

409-
s.assertSQLMockExpectations()
410403
s.Require().NotNil(err)
411404
s.Require().Equal("adding status message: mocked add status message error", err.Error())
405+
s.assertSQLMockExpectations()
412406
}
413407

414408
func (s *InstancesTestSuite) TestUpdateInstance() {
415-
instance, err := s.Store.UpdateInstance(context.Background(), s.Fixtures.Instances[0].ID, s.Fixtures.UpdateInstanceParams)
409+
instance, err := s.Store.UpdateInstance(context.Background(), s.Fixtures.Instances[0].Name, s.Fixtures.UpdateInstanceParams)
416410

417411
s.Require().Nil(err)
418412
s.Require().Equal(s.Fixtures.UpdateInstanceParams.ProviderID, instance.ProviderID)
@@ -424,18 +418,12 @@ func (s *InstancesTestSuite) TestUpdateInstance() {
424418
s.Require().Equal(s.Fixtures.UpdateInstanceParams.CreateAttempt, instance.CreateAttempt)
425419
}
426420

427-
func (s *InstancesTestSuite) TestUpdateInstanceInvalidPoolID() {
428-
_, err := s.Store.UpdateInstance(context.Background(), "dummy-id", params.UpdateInstanceParams{})
429-
430-
s.Require().Equal("updating instance: parsing id: invalid request", err.Error())
431-
}
432-
433421
func (s *InstancesTestSuite) TestUpdateInstanceDBUpdateInstanceErr() {
434422
instance := s.Fixtures.Instances[0]
435423

436424
s.Fixtures.SQLMock.
437-
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE id = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")).
438-
WithArgs(instance.ID).
425+
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE name = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")).
426+
WithArgs(instance.Name).
439427
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(instance.ID))
440428
s.Fixtures.SQLMock.
441429
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `addresses` WHERE `addresses`.`instance_id` = ? AND `addresses`.`deleted_at` IS NULL")).
@@ -455,19 +443,19 @@ func (s *InstancesTestSuite) TestUpdateInstanceDBUpdateInstanceErr() {
455443
WillReturnError(fmt.Errorf("mocked update instance error"))
456444
s.Fixtures.SQLMock.ExpectRollback()
457445

458-
_, err := s.StoreSQLMocked.UpdateInstance(context.Background(), instance.ID, s.Fixtures.UpdateInstanceParams)
446+
_, err := s.StoreSQLMocked.UpdateInstance(context.Background(), instance.Name, s.Fixtures.UpdateInstanceParams)
459447

460-
s.assertSQLMockExpectations()
461448
s.Require().NotNil(err)
462449
s.Require().Equal("updating instance: mocked update instance error", err.Error())
450+
s.assertSQLMockExpectations()
463451
}
464452

465453
func (s *InstancesTestSuite) TestUpdateInstanceDBUpdateAddressErr() {
466454
instance := s.Fixtures.Instances[0]
467455

468456
s.Fixtures.SQLMock.
469-
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE id = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")).
470-
WithArgs(instance.ID).
457+
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE name = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")).
458+
WithArgs(instance.Name).
471459
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(instance.ID))
472460
s.Fixtures.SQLMock.
473461
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `addresses` WHERE `addresses`.`instance_id` = ? AND `addresses`.`deleted_at` IS NULL")).
@@ -501,11 +489,11 @@ func (s *InstancesTestSuite) TestUpdateInstanceDBUpdateAddressErr() {
501489
WillReturnError(fmt.Errorf("update addresses mock error"))
502490
s.Fixtures.SQLMock.ExpectRollback()
503491

504-
_, err := s.StoreSQLMocked.UpdateInstance(context.Background(), instance.ID, s.Fixtures.UpdateInstanceParams)
492+
_, err := s.StoreSQLMocked.UpdateInstance(context.Background(), instance.Name, s.Fixtures.UpdateInstanceParams)
505493

506-
s.assertSQLMockExpectations()
507494
s.Require().NotNil(err)
508495
s.Require().Equal("updating addresses: update addresses mock error", err.Error())
496+
s.assertSQLMockExpectations()
509497
}
510498

511499
func (s *InstancesTestSuite) TestListPoolInstances() {

‎runner/metadata.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,11 @@ func (r *Runner) GetInstanceGithubRegistrationToken(ctx context.Context) (string
166166
TokenFetched: &tokenFetched,
167167
}
168168

169-
if _, err := r.store.UpdateInstance(r.ctx, instance.ID, updateParams); err != nil {
169+
if _, err := r.store.UpdateInstance(r.ctx, instance.Name, updateParams); err != nil {
170170
return "", errors.Wrap(err, "setting token_fetched for instance")
171171
}
172172

173-
if err := r.store.AddInstanceEvent(ctx, instance.ID, params.FetchTokenEvent, params.EventInfo, "runner registration token was retrieved"); err != nil {
173+
if err := r.store.AddInstanceEvent(ctx, instance.Name, params.FetchTokenEvent, params.EventInfo, "runner registration token was retrieved"); err != nil {
174174
return "", errors.Wrap(err, "recording event")
175175
}
176176

‎runner/pool/pool.go‎

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -745,20 +745,6 @@ func (r *basePoolManager) waitForErrorGroupOrContextCancelled(g *errgroup.Group)
745745
}
746746
}
747747

748-
func (r *basePoolManager) fetchInstance(runnerName string) (params.Instance, error) {
749-
runner, err := r.store.GetInstanceByName(r.ctx, runnerName)
750-
if err != nil {
751-
return params.Instance{}, errors.Wrap(err, "fetching instance")
752-
}
753-
754-
_, err = r.GetPoolByID(runner.PoolID)
755-
if err != nil {
756-
return params.Instance{}, errors.Wrap(err, "fetching pool")
757-
}
758-
759-
return runner, nil
760-
}
761-
762748
func (r *basePoolManager) setInstanceRunnerStatus(runnerName string, status params.RunnerStatus) (params.Instance, error) {
763749
updateParams := params.UpdateInstanceParams{
764750
RunnerStatus: status,
@@ -772,12 +758,7 @@ func (r *basePoolManager) setInstanceRunnerStatus(runnerName string, status para
772758
}
773759

774760
func (r *basePoolManager) updateInstance(runnerName string, update params.UpdateInstanceParams) (params.Instance, error) {
775-
runner, err := r.fetchInstance(runnerName)
776-
if err != nil {
777-
return params.Instance{}, errors.Wrap(err, "fetching instance")
778-
}
779-
780-
instance, err := r.store.UpdateInstance(r.ctx, runner.ID, update)
761+
instance, err := r.store.UpdateInstance(r.ctx, runnerName, update)
781762
if err != nil {
782763
return params.Instance{}, errors.Wrap(err, "updating runner state")
783764
}
@@ -980,7 +961,7 @@ func (r *basePoolManager) addInstanceToProvider(instance params.Instance) error
980961
}
981962

982963
updateInstanceArgs := r.updateArgsFromProviderInstance(providerInstance)
983-
if _, err := r.store.UpdateInstance(r.ctx, instance.ID, updateInstanceArgs); err != nil {
964+
if _, err := r.store.UpdateInstance(r.ctx, instance.Name, updateInstanceArgs); err != nil {
984965
return errors.Wrap(err, "updating instance")
985966
}
986967
return nil

‎runner/runner.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -870,12 +870,12 @@ func (r *Runner) ListAllInstances(ctx context.Context) ([]params.Instance, error
870870
}
871871

872872
func (r *Runner) AddInstanceStatusMessage(ctx context.Context, param params.InstanceUpdateMessage) error {
873-
instanceID := auth.InstanceID(ctx)
874-
if instanceID == "" {
873+
instanceName := auth.InstanceName(ctx)
874+
if instanceName == "" {
875875
return runnerErrors.ErrUnauthorized
876876
}
877877

878-
if err := r.store.AddInstanceEvent(ctx, instanceID, params.StatusEvent, params.EventInfo, param.Message); err != nil {
878+
if err := r.store.AddInstanceEvent(ctx, instanceName, params.StatusEvent, params.EventInfo, param.Message); err != nil {
879879
return errors.Wrap(err, "adding status update")
880880
}
881881

@@ -887,17 +887,17 @@ func (r *Runner) AddInstanceStatusMessage(ctx context.Context, param params.Inst
887887
updateParams.AgentID = *param.AgentID
888888
}
889889

890-
if _, err := r.store.UpdateInstance(r.ctx, instanceID, updateParams); err != nil {
890+
if _, err := r.store.UpdateInstance(r.ctx, instanceName, updateParams); err != nil {
891891
return errors.Wrap(err, "updating runner agent ID")
892892
}
893893

894894
return nil
895895
}
896896

897897
func (r *Runner) UpdateSystemInfo(ctx context.Context, param params.UpdateSystemInfoParams) error {
898-
instanceID := auth.InstanceID(ctx)
899-
if instanceID == "" {
900-
slog.ErrorContext(ctx, "missing instance ID")
898+
instanceName := auth.InstanceName(ctx)
899+
if instanceName == "" {
900+
slog.ErrorContext(ctx, "missing instance name")
901901
return runnerErrors.ErrUnauthorized
902902
}
903903

@@ -915,7 +915,7 @@ func (r *Runner) UpdateSystemInfo(ctx context.Context, param params.UpdateSystem
915915
updateParams.AgentID = *param.AgentID
916916
}
917917

918-
if _, err := r.store.UpdateInstance(r.ctx, instanceID, updateParams); err != nil {
918+
if _, err := r.store.UpdateInstance(r.ctx, instanceName, updateParams); err != nil {
919919
return errors.Wrap(err, "updating runner system info")
920920
}
921921

0 commit comments

Comments
 (0)