Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: delete triggeredAt for AutoOps #1179

Merged
merged 11 commits into from
Aug 16, 2024
Merged

chore: delete triggeredAt for AutoOps #1179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9dac685
chore: delete triggeredAt
kakcy Aug 1, 2024
e6c4d79
test: fix test
kakcy Aug 1, 2024
25a8c25
test: fix test
kakcy Aug 1, 2024
80b30bb
chore: add AutoOpsRule.IsCompleted
kakcy Aug 5, 2024
433675f
chore: reformat
kakcy Aug 5, 2024
a03d6c7
chore: reformat
kakcy Aug 5, 2024
49c1e65
chore: revert IsCompleted
kakcy Aug 6, 2024
ab36dfa
chore: make proto-all
kakcy Aug 7, 2024
06331d5
chore: revert value.yaml
kakcy Aug 15, 2024
7fe1124
chore: delete triggered_at column
kakcy Aug 15, 2024
c541c1b
chore: revert value.yaml
kakcy Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions manifests/bucketeer/charts/web/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,3 +137,4 @@ ingress:
name: web
host:
staticIPName:

4 changes: 2 additions & 2 deletions pkg/autoops/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ func (s *AutoOpsService) UpdateAutoOpsRule(
return err
}

if autoOpsRule.AlreadyTriggered() || autoOpsRule.IsFinished() || autoOpsRule.IsStopped() {
if autoOpsRule.IsFinished() || autoOpsRule.IsStopped() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a new method IsCompleted() to the domain?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isCompleted will be confusing IMHO.
Because the complete and finish have the same meaning in this case.
It will be easier to understand and leave it as it is. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cre8ivejp @kentakozuka

It may be confusing how to use it.
Add deleted to the condition,
How about changing it to IsInactive()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kakcy, the issue is not the function name. By creating IsInactive or isCompleted, we are creating a new state that doesn't exist in the auto_ops_rule domain layer, which is confusing because completed and finished words have the same meaning in this case.
So, my suggestion is to leave it as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cre8ivejp @kentakozuka
Since there were multiple locations that made the same judgment, we decided that it would be better to add a new one to improve the readability of the code.

However, as you pointed out, IsCompleted() does not exist in AutoOpsStatus, and having a function that determines a similar status causes confusion when used, so I will discontinue it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted IsCompleted().
1aae01c

dt, err := statusAutoOpsRuleCompleted.WithDetails(&errdetails.LocalizedMessage{
Locale: localizer.GetLocale(),
Message: localizer.MustLocalize(locale.InvalidArgumentError),
Expand Down Expand Up @@ -1549,7 +1549,7 @@ func (s *AutoOpsService) checkIfHasAlreadyTriggered(
}
return false, dt.Err()
}
if autoOpsRule.AlreadyTriggered() || autoOpsRule.IsFinished() || autoOpsRule.IsStopped() || autoOpsRule.Deleted {
if autoOpsRule.IsFinished() || autoOpsRule.IsStopped() || autoOpsRule.Deleted {
s.logger.Warn(
"Auto Ops Rule already triggered",
log.FieldsFromImcomingContext(ctx).AddFields(
Expand Down
27 changes: 6 additions & 21 deletions pkg/autoops/command/auto_ops_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ func (h *autoOpsRuleCommandHandler) Handle(ctx context.Context, cmd Command) err
return h.changeOpsType(ctx, c)
case *proto.DeleteAutoOpsRuleCommand:
return h.delete(ctx, c)
case *proto.ChangeAutoOpsRuleTriggeredAtCommand:
return h.changeTriggeredAt(ctx, c)
case *proto.AddOpsEventRateClauseCommand:
return h.addOpsEventRateClause(ctx, c)
case *proto.ChangeOpsEventRateClauseCommand:
Expand All @@ -84,13 +82,12 @@ func (h *autoOpsRuleCommandHandler) Handle(ctx context.Context, cmd Command) err

func (h *autoOpsRuleCommandHandler) create(ctx context.Context, cmd *proto.CreateAutoOpsRuleCommand) error {
return h.send(ctx, eventproto.Event_AUTOOPS_RULE_CREATED, &eventproto.AutoOpsRuleCreatedEvent{
FeatureId: h.autoOpsRule.FeatureId,
OpsType: h.autoOpsRule.OpsType,
Clauses: h.autoOpsRule.Clauses,
TriggeredAt: h.autoOpsRule.TriggeredAt,
CreatedAt: h.autoOpsRule.CreatedAt,
UpdatedAt: h.autoOpsRule.UpdatedAt,
OpsStatus: h.autoOpsRule.AutoOpsStatus,
FeatureId: h.autoOpsRule.FeatureId,
OpsType: h.autoOpsRule.OpsType,
Clauses: h.autoOpsRule.Clauses,
CreatedAt: h.autoOpsRule.CreatedAt,
UpdatedAt: h.autoOpsRule.UpdatedAt,
OpsStatus: h.autoOpsRule.AutoOpsStatus,
})
}

Expand All @@ -114,18 +111,6 @@ func (h *autoOpsRuleCommandHandler) delete(ctx context.Context, cmd *proto.Delet
return h.send(ctx, eventproto.Event_AUTOOPS_RULE_DELETED, &eventproto.AutoOpsRuleDeletedEvent{})
}

func (h *autoOpsRuleCommandHandler) changeTriggeredAt(
ctx context.Context,
cmd *proto.ChangeAutoOpsRuleTriggeredAtCommand,
) error {
h.autoOpsRule.SetTriggeredAt()
return h.send(
ctx,
eventproto.Event_AUTOOPS_RULE_TRIGGERED_AT_CHANGED,
&eventproto.AutoOpsRuleTriggeredAtChangedEvent{},
)
}

func (h *autoOpsRuleCommandHandler) changeAutoOpsStatus(
ctx context.Context,
cmd *proto.ChangeAutoOpsStatusCommand,
Expand Down
23 changes: 0 additions & 23 deletions pkg/autoops/command/auto_ops_rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,29 +78,6 @@ func TestDelete(t *testing.T) {
}
}

func TestChangeTriggeredAt(t *testing.T) {
mockController := gomock.NewController(t)
defer mockController.Finish()
patterns := []*struct {
expected error
}{
{
expected: nil,
},
}
for _, p := range patterns {
m := publishermock.NewMockPublisher(mockController)
a := newAutoOpsRule(t)
h := newAutoOpsRuleCommandHandler(t, m, a)
if p.expected == nil {
m.EXPECT().Publish(gomock.Any(), gomock.Any()).Return(nil)
}
cmd := &proto.ChangeAutoOpsRuleTriggeredAtCommand{}
err := h.Handle(context.Background(), cmd)
assert.Equal(t, p.expected, err)
}
}

func TestChangeAutoOpsStatus(t *testing.T) {
mockController := gomock.NewController(t)
defer mockController.Finish()
Expand Down
24 changes: 0 additions & 24 deletions pkg/autoops/domain/auto_ops_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,25 +108,10 @@ func (a *AutoOpsRule) SetDeleted() {
a.AutoOpsRule.UpdatedAt = time.Now().Unix()
}

// TODO: Remove this function after auto ops migration.
// Deprecated
func (a *AutoOpsRule) SetTriggeredAt() {
now := time.Now().Unix()
a.AutoOpsRule.TriggeredAt = now
a.SetFinished()
a.AutoOpsRule.UpdatedAt = now
}

func (a *AutoOpsRule) SetFinished() {
a.SetAutoOpsStatus(proto.AutoOpsStatus_FINISHED)
}

// TODO: Remove this function after auto ops migration.
// Deprecated
func (a *AutoOpsRule) AlreadyTriggered() bool {
return a.TriggeredAt > 0 || a.AutoOpsStatus == proto.AutoOpsStatus_FINISHED
}

func (a *AutoOpsRule) IsFinished() bool {
return a.AutoOpsStatus == proto.AutoOpsStatus_FINISHED
}
Expand All @@ -138,17 +123,12 @@ func (a *AutoOpsRule) IsStopped() bool {
func (a *AutoOpsRule) SetOpsType(opsType proto.OpsType) {
a.AutoOpsRule.OpsType = opsType
a.AutoOpsRule.UpdatedAt = time.Now().Unix()
a.AutoOpsRule.TriggeredAt = 0
}

func (a *AutoOpsRule) SetAutoOpsStatus(status proto.AutoOpsStatus) {
now := time.Now().Unix()
a.AutoOpsRule.AutoOpsStatus = status
a.AutoOpsRule.UpdatedAt = now
// TODO: Remove this function after auto ops migration.
if status == proto.AutoOpsStatus_FINISHED {
a.AutoOpsRule.TriggeredAt = now
}
}

func (a *AutoOpsRule) AddOpsEventRateClause(oerc *proto.OpsEventRateClause) (*proto.Clause, error) {
Expand All @@ -161,7 +141,6 @@ func (a *AutoOpsRule) AddOpsEventRateClause(oerc *proto.OpsEventRateClause) (*pr
return nil, err
}
a.AutoOpsRule.UpdatedAt = time.Now().Unix()
a.AutoOpsRule.TriggeredAt = 0
return clause, nil
}

Expand Down Expand Up @@ -240,12 +219,10 @@ func (a *AutoOpsRule) ChangeDatetimeClause(id string, dc *proto.DatetimeClause)
return err
}
a.AutoOpsRule.UpdatedAt = time.Now().Unix()
a.AutoOpsRule.TriggeredAt = 0
return nil
}

func (a *AutoOpsRule) changeClause(id string, mc pb.Message, actionType proto.ActionType) error {
a.AutoOpsRule.TriggeredAt = 0
for _, c := range a.Clauses {
if c.Id == id {
clause, err := ptypes.MarshalAny(mc)
Expand All @@ -265,7 +242,6 @@ func (a *AutoOpsRule) DeleteClause(id string) error {
return errClauseEmpty
}
a.AutoOpsRule.UpdatedAt = time.Now().Unix()
a.AutoOpsRule.TriggeredAt = 0
var clauses []*proto.Clause
for i, c := range a.Clauses {
if c.Id == id {
Expand Down
31 changes: 0 additions & 31 deletions pkg/autoops/domain/auto_ops_rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ func TestNewAutoOpsRule(t *testing.T) {
},
CreatedAt: time.Now().Unix(),
UpdatedAt: time.Now().Unix(),
TriggeredAt: 0,
Deleted: false,
AutoOpsStatus: autoopsproto.AutoOpsStatus_WAITING,
}},
Expand Down Expand Up @@ -109,7 +108,6 @@ func TestNewAutoOpsRule(t *testing.T) {
},
CreatedAt: time.Now().Unix(),
UpdatedAt: time.Now().Unix(),
TriggeredAt: 0,
Deleted: false,
AutoOpsStatus: autoopsproto.AutoOpsStatus_WAITING,
}},
Expand Down Expand Up @@ -141,7 +139,6 @@ func TestNewAutoOpsRule(t *testing.T) {
},
CreatedAt: time.Now().Unix(),
UpdatedAt: time.Now().Unix(),
TriggeredAt: 0,
Deleted: false,
AutoOpsStatus: autoopsproto.AutoOpsStatus_WAITING,
}},
Expand Down Expand Up @@ -177,7 +174,6 @@ func TestNewAutoOpsRule(t *testing.T) {
},
CreatedAt: time.Now().Unix(),
UpdatedAt: time.Now().Unix(),
TriggeredAt: 0,
Deleted: false,
AutoOpsStatus: autoopsproto.AutoOpsStatus_WAITING,
}},
Expand All @@ -199,7 +195,6 @@ func TestNewAutoOpsRule(t *testing.T) {
assert.Equal(t, p.expected.AutoOpsStatus, aor.AutoOpsStatus)
assert.Equal(t, p.expected.CreatedAt, aor.CreatedAt)
assert.Equal(t, p.expected.UpdatedAt, aor.UpdatedAt)
assert.Equal(t, p.expected.TriggeredAt, aor.TriggeredAt)
assert.Equal(t, p.expected.Deleted, aor.Deleted)

for i, c := range aor.Clauses {
Expand All @@ -219,35 +214,16 @@ func TestSetDeleted(t *testing.T) {
assert.Equal(t, true, aor.Deleted)
}

func TestSetTriggeredAt(t *testing.T) {
t.Parallel()
aor := createAutoOpsRule(t)
aor.SetTriggeredAt()
assert.NotZero(t, aor.TriggeredAt)
assert.Equal(t, autoopsproto.AutoOpsStatus_FINISHED, aor.AutoOpsStatus)
}

func TestAlreadyTriggeredAt(t *testing.T) {
t.Parallel()
aor := createAutoOpsRule(t)
assert.False(t, aor.AlreadyTriggered())
aor.SetTriggeredAt()
assert.True(t, aor.AlreadyTriggered())
}

func TestSetOpsType(t *testing.T) {
t.Parallel()
aor := createAutoOpsRule(t)
aor.TriggeredAt = 1
aor.SetOpsType(autoopsproto.OpsType_DISABLE_FEATURE)
assert.Equal(t, autoopsproto.OpsType_DISABLE_FEATURE, aor.OpsType)
assert.Zero(t, aor.TriggeredAt)
}

func TestAddOpsEventRateClause(t *testing.T) {
t.Parallel()
aor := createAutoOpsRule(t)
aor.TriggeredAt = 1
l := len(aor.Clauses)
c := &autoopsproto.OpsEventRateClause{
GoalId: "goalid01",
Expand All @@ -262,7 +238,6 @@ func TestAddOpsEventRateClause(t *testing.T) {
assert.NotEmpty(t, aor.Clauses[l].Id)
eventRateClause, err := aor.unmarshalOpsEventRateClause(aor.Clauses[l])
require.NoError(t, err)
assert.Zero(t, aor.TriggeredAt)

assert.Equal(t, c.GoalId, eventRateClause.GoalId)
assert.Equal(t, c.MinCount, eventRateClause.MinCount)
Expand Down Expand Up @@ -308,7 +283,6 @@ func TestAddDatetimeClause(t *testing.T) {
func TestChangeOpsEventRateClause(t *testing.T) {
t.Parallel()
aor := createAutoOpsRule(t)
aor.TriggeredAt = 1
l := len(aor.Clauses)
c := &autoopsproto.OpsEventRateClause{
GoalId: "goalid01",
Expand All @@ -321,14 +295,12 @@ func TestChangeOpsEventRateClause(t *testing.T) {
assert.Equal(t, l, len(aor.Clauses))
eventRateClause, err := aor.unmarshalOpsEventRateClause(aor.Clauses[0])
require.NoError(t, err)
assert.Zero(t, aor.TriggeredAt)
assert.Equal(t, c.GoalId, eventRateClause.GoalId)
}

func TestChangeDatetimeClause(t *testing.T) {
t.Parallel()
aor := createAutoOpsRule(t)
aor.TriggeredAt = 1
l := len(aor.Clauses)
c := &autoopsproto.DatetimeClause{
Time: 1,
Expand All @@ -341,7 +313,6 @@ func TestChangeDatetimeClause(t *testing.T) {
dc, err := aor.unmarshalDatetimeClause(aor.Clauses[0])
require.NoError(t, err)
assert.Equal(t, c.Time, dc.Time)
assert.Zero(t, aor.TriggeredAt)
assert.Equal(t, autoopsproto.AutoOpsStatus_WAITING, aor.AutoOpsStatus)

c1 := &autoopsproto.DatetimeClause{
Expand Down Expand Up @@ -382,7 +353,6 @@ func TestChangeDatetimeClause(t *testing.T) {
func TestDeleteClause(t *testing.T) {
t.Parallel()
aor := createAutoOpsRule(t)
aor.TriggeredAt = 1
l := len(aor.Clauses)
c := &autoopsproto.OpsEventRateClause{
GoalId: "goalid01",
Expand All @@ -397,7 +367,6 @@ func TestDeleteClause(t *testing.T) {
err = aor.DeleteClause(aor.Clauses[0].Id)
require.NoError(t, err)
assert.Equal(t, l, len(aor.Clauses))
assert.Zero(t, aor.TriggeredAt)
assert.Equal(t, addClause.Id, aor.Clauses[0].Id)
assert.Equal(t, autoopsproto.AutoOpsStatus_WAITING, aor.AutoOpsStatus)
}
Expand Down
4 changes: 0 additions & 4 deletions pkg/autoops/storage/v2/auto_ops_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ func (s *autoOpsRuleStorage) CreateAutoOpsRule(
e.FeatureId,
int32(e.OpsType),
mysql.JSONObject{Val: e.Clauses},
e.TriggeredAt,
e.CreatedAt,
e.UpdatedAt,
e.Deleted,
Expand All @@ -102,7 +101,6 @@ func (s *autoOpsRuleStorage) UpdateAutoOpsRule(
e.FeatureId,
int32(e.OpsType),
mysql.JSONObject{Val: e.Clauses},
e.TriggeredAt,
e.CreatedAt,
e.UpdatedAt,
e.Deleted,
Expand Down Expand Up @@ -139,7 +137,6 @@ func (s *autoOpsRuleStorage) GetAutoOpsRule(
&autoOpsRule.FeatureId,
&opsType,
&mysql.JSONObject{Val: &autoOpsRule.Clauses},
&autoOpsRule.TriggeredAt,
&autoOpsRule.CreatedAt,
&autoOpsRule.UpdatedAt,
&autoOpsRule.Deleted,
Expand Down Expand Up @@ -179,7 +176,6 @@ func (s *autoOpsRuleStorage) ListAutoOpsRules(
&autoOpsRule.FeatureId,
&opsType,
&mysql.JSONObject{Val: &autoOpsRule.Clauses},
&autoOpsRule.TriggeredAt,
&autoOpsRule.CreatedAt,
&autoOpsRule.UpdatedAt,
&autoOpsRule.Deleted,
Expand Down
3 changes: 1 addition & 2 deletions pkg/autoops/storage/v2/sql/auto_ops_rule/insert_auto_ops_rule.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@ INSERT INTO auto_ops_rule (
feature_id,
ops_type,
clauses,
triggered_at,
created_at,
updated_at,
deleted,
status,
environment_namespace
) VALUES (
?, ?, ?, ?, ?, ?, ?, ?, ?, ?
?, ?, ?, ?, ?, ?, ?, ?, ?
)
1 change: 0 additions & 1 deletion pkg/autoops/storage/v2/sql/auto_ops_rule/select_auto_ops_rule.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ SELECT
feature_id,
ops_type,
clauses,
triggered_at,
created_at,
updated_at,
deleted,
Expand Down
1 change: 0 additions & 1 deletion pkg/autoops/storage/v2/sql/auto_ops_rule/select_auto_ops_rules.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ SELECT
feature_id,
ops_type,
clauses,
triggered_at,
created_at,
updated_at,
deleted,
Expand Down
1 change: 0 additions & 1 deletion pkg/autoops/storage/v2/sql/auto_ops_rule/update_auto_ops_rule.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ SET
feature_id = ?,
ops_type = ?,
clauses = ?,
triggered_at = ?,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted it.

created_at = ?,
updated_at = ?,
deleted = ?,
Expand Down
2 changes: 1 addition & 1 deletion pkg/batch/jobs/opsevent/datetime_watcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func (w *datetimeWatcher) Run(ctx context.Context) (lastErr error) {
}
for _, a := range autoOpsRules {
aor := &autoopsdomain.AutoOpsRule{AutoOpsRule: a}
if aor.AlreadyTriggered() || aor.IsStopped() || aor.IsFinished() {
if aor.IsFinished() || aor.IsStopped() {
continue
}
executeClauseID, err := w.getExecuteClauseId(ctx, env.Id, aor)
Expand Down
Loading
Loading