Skip to content

Commit

Permalink
Remove Create function from nats store
Browse files Browse the repository at this point in the history
It is unused, vestigial code, and provides no additional functionality from "CreateMultiple"
  • Loading branch information
jakeschuurmans committed Aug 26, 2024
1 parent af22b4b commit 32d1b62
Show file tree
Hide file tree
Showing 12 changed files with 52 additions and 91 deletions.
4 changes: 0 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -541,10 +541,6 @@ github.com/mattn/go-sqlite3 v1.14.22/go.mod h1:Uh1q+B4BYcTPb+yiD3kU8Ct7aC0hY9fxU
github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0=
github.com/metal-toolbox/fleetdb v1.19.3 h1:Dk7MVi5rS42a4OI46Ye/etg8R5hm1iajaI4U0k1GSec=
github.com/metal-toolbox/fleetdb v1.19.3/go.mod h1:v9agAzF7BhBBjA4rY+H2eZzQaBTEXO4b/Qikn4jmA7A=
github.com/metal-toolbox/rivets v1.3.1-0.20240819072158-b0e2d577ffb2 h1:oLVmapAbFYZjt+BMsLpkfSGEV9f+PZyS1Z0ZDOtzbJs=
github.com/metal-toolbox/rivets v1.3.1-0.20240819072158-b0e2d577ffb2/go.mod h1:i78a1x0w2uNyMlvUgyO6ST552u9wV2Xa4+A73oA4WJY=
github.com/metal-toolbox/rivets v1.3.2-0.20240821095555-56f1bf4b1a29 h1:m8suYpZ7V3IHwiEUFmcjo1KD19+ir6iXmY1a9rQo7Nk=
github.com/metal-toolbox/rivets v1.3.2-0.20240821095555-56f1bf4b1a29/go.mod h1:i78a1x0w2uNyMlvUgyO6ST552u9wV2Xa4+A73oA4WJY=
github.com/metal-toolbox/rivets v1.3.2 h1:wazODpX94E2IbjuqEAmARl1r9TZE0NS1vBiGSpBW/tE=
github.com/metal-toolbox/rivets v1.3.2/go.mod h1:i78a1x0w2uNyMlvUgyO6ST552u9wV2Xa4+A73oA4WJY=
github.com/metal-toolbox/rivets v1.3.3 h1:bHd2HRh/uc3zb2bughpuidW2lUQOXrFRRDWBWwqMQWw=
Expand Down
2 changes: 1 addition & 1 deletion internal/orchestrator/updates.go
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,7 @@ func (o *Orchestrator) reconcileActiveConditionRecords(ctx context.Context) {
if err != nil {
// create record if it doesn't exist
if errors.Is(err, store.ErrConditionNotFound) {
if errCreate := o.repository.CreateMultiple(ctx, cond.Target, o.facility, cond); errCreate != nil {
if errCreate := o.repository.Create(ctx, cond.Target, o.facility, cond); errCreate != nil {
le.WithError(errCreate).Warn("reconciler condition record create")
}

Expand Down
4 changes: 2 additions & 2 deletions internal/orchestrator/updates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ func TestActiveConditionsToReconcile_Creates(t *testing.T) {

ctx := context.Background()
for _, cond := range tc.conditions {
err := o.repository.CreateMultiple(ctx, cond.Target, o.facility, cond)
err := o.repository.Create(ctx, cond.Target, o.facility, cond)
require.NoError(t, err)
}

Expand Down Expand Up @@ -379,7 +379,7 @@ func TestActiveConditionsToReconcile_Updates(t *testing.T) {
// active-conditions handle
acKV := newCleanActiveConditionsKV(t)

if err := o.repository.CreateMultiple(ctx, sid, o.facility, fwcond, invcond); err != nil {
if err := o.repository.Create(ctx, sid, o.facility, fwcond, invcond); err != nil {
t.Fatal(err)
}

Expand Down
4 changes: 2 additions & 2 deletions internal/store/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ type Repository interface {
// @serverID: required
GetActiveCondition(ctx context.Context, serverID uuid.UUID) (*rctypes.Condition, error)

// Create a condition record that encapsulates a unit of work encompassing multiple conditions
// Create a condition record that encapsulates a unit of work, which can encompass one or many conditions.
// If you create a condition record with 0 conditions, you don't actually create anything, but
// no error is returned.
CreateMultiple(ctx context.Context, serverID uuid.UUID, facilityCode string, conditions ...*rctypes.Condition) error
Create(ctx context.Context, serverID uuid.UUID, facilityCode string, conditions ...*rctypes.Condition) error

// Update a condition on a server
// @serverID: required
Expand Down
22 changes: 11 additions & 11 deletions internal/store/mock_repository.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 6 additions & 41 deletions internal/store/nats.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,49 +182,14 @@ func (n *natsStore) GetActiveCondition(ctx context.Context, serverID uuid.UUID)
return active, nil
}

// Create a condition on a server.
// @id: required
// @condition: required
//
// Note: it is up to the caller to validate the condition payload and to check
// any existing condition before creating.
func (n *natsStore) Create(ctx context.Context, serverID uuid.UUID, condition *rctypes.Condition) error {
_, span := otel.Tracer(pkgName).Start(ctx, "NatsStore.Create")
defer span.End()

le := n.log.WithFields(logrus.Fields{
"serverID": serverID.String(),
"conditionKind": condition.Kind,
"conditionID": condition.ID.String(),
})

state := rctypes.Pending
if condition.State != state {
state = condition.State
}

cr := ConditionRecord{
ID: condition.ID,
State: state,
Conditions: []*rctypes.Condition{
condition,
},
}

_, err := n.bucket.Put(serverID.String(), cr.MustJSON())
if err != nil {
natsError("create")
span.RecordError(err)
le.WithError(err).Warn("writing condition to storage")
}
return err
}

// CreateMultiple crafts a condition-record that is comprised of multiple individual conditions. Unlike Create
// Create one or more conditions on a server to define a unit of work to be done on a server.
// it checks for an existing, active ConditionRecord prior to queuing the new one, and will return an error if
// it finds one.
func (n *natsStore) CreateMultiple(ctx context.Context, serverID uuid.UUID, facilityCode string, work ...*rctypes.Condition) error {
otelCtx, span := otel.Tracer(pkgName).Start(ctx, "NatsStore.CreateMultiple")
// @serverID: required
// @facilityCode: required
// @work: optional
func (n *natsStore) Create(ctx context.Context, serverID uuid.UUID, facilityCode string, work ...*rctypes.Condition) error {
otelCtx, span := otel.Tracer(pkgName).Start(ctx, "NatsStore.Create")
defer span.End()

le := n.log.WithFields(logrus.Fields{
Expand Down
20 changes: 10 additions & 10 deletions internal/store/nats_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func TestCreateReadUpdate(t *testing.T) {
require.Nil(t, active)

// add a condition
err = store.CreateMultiple(context.TODO(), serverID, "fc-13", condition)
err = store.Create(context.TODO(), serverID, "fc-13", condition)
require.NoError(t, err)

active, err = store.GetActiveCondition(context.TODO(), serverID)
Expand Down Expand Up @@ -147,7 +147,7 @@ func TestCreateReadUpdate(t *testing.T) {
}

// Given a conditionRecord with multiple conditions, walk through some common
// scenarios around CreateMultiple/Update/Get/GetActive
// scenarios around Create/Update/Get/GetActive
func TestMultipleConditionUpdate(t *testing.T) {
t.Parallel()

Expand All @@ -161,20 +161,20 @@ func TestMultipleConditionUpdate(t *testing.T) {
t.Run("create multiple sanity checks", func(t *testing.T) {
serverID := uuid.New()

err := store.CreateMultiple(context.TODO(), serverID, facility)
err := store.Create(context.TODO(), serverID, facility)
require.NoError(t, err, "created multiple with nil work")

work := []*rctypes.Condition{
&rctypes.Condition{
{
Kind: rctypes.Kind("first"),
State: rctypes.Pending,
},
}

err = store.CreateMultiple(context.TODO(), serverID, facility, work...)
err = store.Create(context.TODO(), serverID, facility, work...)
require.NoError(t, err, "created multiple on idle server with work")

err = store.CreateMultiple(context.TODO(), serverID, facility, work...)
err = store.Create(context.TODO(), serverID, facility, work...)
require.ErrorIs(t, err, ErrActiveCondition, "created multiple on busy server with work")

})
Expand All @@ -193,8 +193,8 @@ func TestMultipleConditionUpdate(t *testing.T) {
second,
}

err := store.CreateMultiple(context.TODO(), serverID, facility, work...)
require.NoError(t, err, "CreateMultiple")
err := store.Create(context.TODO(), serverID, facility, work...)
require.NoError(t, err, "Create")

active, err := store.GetActiveCondition(context.TODO(), serverID)
require.NoError(t, err, "GetActiveCondition I")
Expand Down Expand Up @@ -243,8 +243,8 @@ func TestMultipleConditionUpdate(t *testing.T) {
second,
}

err := store.CreateMultiple(context.TODO(), serverID, facility, work...)
require.NoError(t, err, "CreateMultiple")
err := store.Create(context.TODO(), serverID, facility, work...)
require.NoError(t, err, "Create")

active, err := store.GetActiveCondition(context.TODO(), serverID)
require.NoError(t, err, "GetActiveCondition I")
Expand Down
12 changes: 6 additions & 6 deletions pkg/api/v1/conditions/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ func TestConditionCreate(t *testing.T) {
).Once()

r.On(
"CreateMultiple",
"Create",
mock.Anything,
serverID,
mock.Anything,
Expand Down Expand Up @@ -359,7 +359,7 @@ func TestFirmwareInstall(t *testing.T) {
FirmwareSetID: fwset.UUID,
},
mockStore: func(r *store.MockRepository) {
r.On("CreateMultiple", mock.Anything, serverID, "facility", mock.Anything, mock.Anything).
r.On("Create", mock.Anything, serverID, "facility", mock.Anything, mock.Anything).
Return(nil).Once()

},
Expand All @@ -384,7 +384,7 @@ func TestFirmwareInstall(t *testing.T) {
FirmwareSetID: fwset.UUID,
},
mockStore: func(r *store.MockRepository) {
r.On("CreateMultiple", mock.Anything, serverID, "facility", mock.Anything, mock.Anything).
r.On("Create", mock.Anything, serverID, "facility", mock.Anything, mock.Anything).
Return(nil).Once()
},
mockFleetDB: func(r *fleetdb.MockFleetDB) {
Expand Down Expand Up @@ -413,7 +413,7 @@ func TestFirmwareInstall(t *testing.T) {
Return(fwset, nil).Once()
},
mockStore: func(r *store.MockRepository) {
r.On("CreateMultiple", mock.Anything, serverID, "facility", mock.Anything, mock.Anything).
r.On("Create", mock.Anything, serverID, "facility", mock.Anything, mock.Anything).
Return(fmt.Errorf("%w:%s", store.ErrActiveCondition, "pound sand")).Once()
},
expectResponse: func() *v1types.ServerResponse {
Expand Down Expand Up @@ -520,7 +520,7 @@ func TestServerEnroll(t *testing.T) {
},
mockStore: func(r *store.MockRepository) {
// expect valid payload
r.On("CreateMultiple",
r.On("Create",
mock.Anything,
serverID,
mock.Anything,
Expand Down Expand Up @@ -664,7 +664,7 @@ func TestServerEnrollEmptyUUID(t *testing.T) {
Return(rollback, nil).
Once()

tester.repository.On("CreateMultiple",
tester.repository.On("Create",
mock.Anything,
mock.Anything,
mock.Anything,
Expand Down
4 changes: 2 additions & 2 deletions pkg/api/v1/conditions/routes/firmware_validation_handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ func (r *Routes) validateFirmware(c *gin.Context) (int, *v1types.ServerResponse)

conds := firmwareValidationConditions(fvr)
// XXX: This is lifted from the firmwareInstall api code. Consider abstracting a utility method for
// CreateMultiple and publishCondition.
if err = r.repository.CreateMultiple(otelCtx, fvr.ServerID, facilityCode, conds.Conditions...); err != nil {
// Create and publishCondition.
if err = r.repository.Create(otelCtx, fvr.ServerID, facilityCode, conds.Conditions...); err != nil {
if errors.Is(err, store.ErrActiveCondition) {
return http.StatusConflict, &v1types.ServerResponse{
Message: err.Error(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ func TestValidateFirmware(t *testing.T) {
require.NoError(t, err, "creating validation request")

fleetdb.On("GetServer", mock.Anything, mock.Anything).Return(srv, nil).Twice()
repo.On("CreateMultiple", mock.Anything, mock.Anything, "fc13", mock.Anything, mock.Anything).
repo.On("Create", mock.Anything, mock.Anything, "fc13", mock.Anything, mock.Anything).
Return(store.ErrActiveCondition).Once()
repo.On("CreateMultiple", mock.Anything, mock.Anything, "fc13", mock.Anything, mock.Anything).
repo.On("Create", mock.Anything, mock.Anything, "fc13", mock.Anything, mock.Anything).
Return(errors.New("pound sand")).Once()

req, err := http.NewRequestWithContext(context.TODO(), http.MethodPost, fwValidationUrl, bytes.NewReader(fvr))
Expand Down Expand Up @@ -109,7 +109,7 @@ func TestValidateFirmware(t *testing.T) {

fleetdb.On("GetServer", mock.Anything, mock.Anything).Return(srv, nil).Once()

repo.On("CreateMultiple", mock.Anything, mock.Anything, "fc13", mock.Anything, mock.Anything).
repo.On("Create", mock.Anything, mock.Anything, "fc13", mock.Anything, mock.Anything).
Return(nil).Once()
repo.On("Update", mock.Anything, srv.ID, mock.Anything).Return(nil).Once()

Expand Down Expand Up @@ -141,7 +141,7 @@ func TestValidateFirmware(t *testing.T) {

fleetdb.On("GetServer", mock.Anything, mock.Anything).Return(srv, nil).Once()

repo.On("CreateMultiple", mock.Anything, mock.Anything, "fc13", mock.Anything, mock.Anything).
repo.On("Create", mock.Anything, mock.Anything, "fc13", mock.Anything, mock.Anything).
Return(nil).Once()

stream.On("Publish", mock.Anything, "fc13.servers.firmwareInstall", mock.Anything).Return(nil).Once()
Expand Down
4 changes: 2 additions & 2 deletions pkg/api/v1/conditions/routes/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ func (r *Routes) firmwareInstall(c *gin.Context) (int, *v1types.ServerResponse)
}

serverConditions := r.firmwareInstallComposite(serverID, fwtp, fwset)
if err = r.repository.CreateMultiple(otelCtx, serverID, facilityCode, serverConditions.Conditions...); err != nil {
if err = r.repository.Create(otelCtx, serverID, facilityCode, serverConditions.Conditions...); err != nil {
if errors.Is(err, store.ErrActiveCondition) {
return http.StatusConflict, &v1types.ServerResponse{
Message: err.Error(),
Expand Down Expand Up @@ -501,7 +501,7 @@ func (r *Routes) firmwareInstallComposite(

func (r *Routes) conditionCreate(otelCtx context.Context, newCondition *rctypes.Condition, serverID uuid.UUID, facilityCode string) (int, *v1types.ServerResponse) {
// Create the new condition
err := r.repository.CreateMultiple(otelCtx, serverID, facilityCode, newCondition)
err := r.repository.Create(otelCtx, serverID, facilityCode, newCondition)
if err != nil {
r.logger.WithError(err).Info("condition create failed")

Expand Down
Loading

0 comments on commit 32d1b62

Please sign in to comment.