chore: delete triggeredAt for AutoOps #1179

kakcy · 2024-08-01T10:37:19Z

No description provided.

kentakozuka

Thanks! left comments. ptal.

kentakozuka · 2024-08-05T02:32:25Z

proto/event/domain/event.proto

@@ -789,7 +789,7 @@ message AutoOpsRuleCreatedEvent {
 string feature_id = 1;
 bucketeer.autoops.OpsType ops_type = 2;
 repeated bucketeer.autoops.Clause clauses = 3;
- int64 triggered_at = 4 [deprecated = true];
+ reserved 4;


Suggested change

reserved 4;

reserved 4; // int64 triggered_at = 4 [deprecated = true];

kentakozuka · 2024-08-05T02:32:52Z

proto/autoops/service.proto

@@ -85,8 +85,7 @@ message UpdateAutoOpsRuleResponse {}
 message ExecuteAutoOpsRequest {
 string environment_namespace = 1;
 string id = 2;
- ChangeAutoOpsRuleTriggeredAtCommand
- change_auto_ops_rule_triggered_at_command = 3 [deprecated = true];
+ reserved 3;


Suggested change

reserved 3;

reserved 3; // ChangeAutoOpsRuleTriggeredAtCommand

change_auto_ops_rule_triggered_at_command = 3 [deprecated = true];

kentakozuka · 2024-08-05T02:33:11Z

proto/autoops/auto_ops_rule.proto

@@ -24,7 +24,7 @@ message AutoOpsRule {
 string feature_id = 2;
 OpsType ops_type = 3;
 repeated Clause clauses = 4;
- int64 triggered_at = 6 [deprecated = true];
+ reserved 6;


Suggested change

reserved 6;

reserved 6; // int64 triggered_at = 6 [deprecated = true];

kentakozuka · 2024-08-05T02:40:37Z

pkg/autoops/api/api.go

@@ -706,7 +706,7 @@ func (s *AutoOpsService) UpdateAutoOpsRule(
 return err
 }

- if autoOpsRule.AlreadyTriggered() || autoOpsRule.IsFinished() || autoOpsRule.IsStopped() {
+ if autoOpsRule.IsFinished() || autoOpsRule.IsStopped() {


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

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?

@cre8ivejp @kentakozuka

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

@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.

@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.

I reverted IsCompleted().
1aae01c

kakcy

I have corrected the points pointed out.
Please check it when you have time.

kakcy · 2024-08-05T05:24:48Z

pkg/autoops/domain/auto_ops_rule.go

@@ -135,20 +120,19 @@ func (a *AutoOpsRule) IsStopped() bool {
 return a.AutoOpsStatus == proto.AutoOpsStatus_STOPPED
 }

+func (a *AutoOpsRule) IsCompleted() bool {


add IsCompleted()

cre8ivejp · 2024-08-05T06:16:50Z

pkg/autoops/storage/v2/auto_ops_rule.go

@@ -179,7 +182,7 @@ func (s *autoOpsRuleStorage) ListAutoOpsRules(
 &autoOpsRule.FeatureId,
 &opsType,
 &mysql.JSONObject{Val: &autoOpsRule.Clauses},
- &autoOpsRule.TriggeredAt,
+ &tmpTriggeredAt, // ToDo: TriggeredAt is deprecated. Remove this line after migration.


By removing this line, we don't need to wait for the migration.

cre8ivejp · 2024-08-05T06:16:54Z

pkg/autoops/storage/v2/auto_ops_rule.go

@@ -139,7 +141,7 @@ func (s *autoOpsRuleStorage) GetAutoOpsRule(
 &autoOpsRule.FeatureId,
 &opsType,
 &mysql.JSONObject{Val: &autoOpsRule.Clauses},
- &autoOpsRule.TriggeredAt,
+ &tmpTriggeredAt, // ToDo: TriggeredAt is deprecated. Remove this line after migration.


By removing this line, we don't need to wait for the migration.

cre8ivejp · 2024-08-05T06:17:31Z

pkg/autoops/storage/v2/auto_ops_rule.go

@@ -102,7 +102,7 @@ func (s *autoOpsRuleStorage) UpdateAutoOpsRule(
 e.FeatureId,
 int32(e.OpsType),
 mysql.JSONObject{Val: e.Clauses},
- e.TriggeredAt,
+ 0, // ToDo: TriggeredAt is deprecated. Remove this line after migration.


By removing this line, we don't need to wait for the migration.

cre8ivejp · 2024-08-05T06:25:13Z

pkg/autoops/storage/v2/auto_ops_rule.go

@@ -75,7 +75,7 @@ func (s *autoOpsRuleStorage) CreateAutoOpsRule(
 e.FeatureId,
 int32(e.OpsType),
 mysql.JSONObject{Val: e.Clauses},
- e.TriggeredAt,
+ 0, // ToDo: TriggeredAt is deprecated. Remove this line after migration.


It will be safer if you alter the triggered_at column and set a default value to zero. Then, you can remove this line without waiting for the migration.

@cre8ivejp
Thank you!
I tried to respond with this commit, please check if there is any problem with the response method.

cre8ivejp · 2024-08-05T08:04:33Z

pkg/autoops/storage/v2/sql/auto_ops_rule/update_auto_ops_rule.sql

@@ -4,7 +4,7 @@ SET
 feature_id = ?,
 ops_type = ?,
 clauses = ?,
- triggered_at = ?,
+ triggered_at = 0,


There is no need to set this to zero.
You can remove this line from the query because the field is not required when updating.

kakcy

I have corrected the points pointed out.
Please check it when you have time.

kakcy · 2024-08-06T02:55:37Z

pkg/autoops/api/api.go

@@ -706,7 +706,7 @@ func (s *AutoOpsService) UpdateAutoOpsRule(
 return err
 }

- if autoOpsRule.AlreadyTriggered() || autoOpsRule.IsFinished() || autoOpsRule.IsStopped() {
+ if autoOpsRule.IsFinished() || autoOpsRule.IsStopped() {


I reverted IsCompleted().
1aae01c

kakcy · 2024-08-06T02:57:58Z

pkg/autoops/storage/v2/sql/auto_ops_rule/update_auto_ops_rule.sql

@@ -4,7 +4,6 @@ SET
 feature_id = ?,
 ops_type = ?,
 clauses = ?,
- triggered_at = ?,


I deleted it.

cre8ivejp

Thank you!

kakcy marked this pull request as ready for review August 5, 2024 02:25

kakcy requested review from cre8ivejp and kentakozuka as code owners August 5, 2024 02:25

kentakozuka reviewed Aug 5, 2024

View reviewed changes

kakcy commented Aug 5, 2024

View reviewed changes

cre8ivejp reviewed Aug 5, 2024

View reviewed changes

kakcy force-pushed the delete-triggered-at-for-auto-ops branch from df69d39 to 1aae01c Compare August 6, 2024 02:27

kakcy commented Aug 6, 2024

View reviewed changes

kakcy requested review from cre8ivejp and kentakozuka August 6, 2024 04:13

kakcy mentioned this pull request Aug 6, 2024

chore: delete triggeredAt for auto_ops_role table #1189

Closed

kakcy force-pushed the delete-triggered-at-for-auto-ops branch from 1aae01c to 7eb8ba5 Compare August 7, 2024 07:38

cre8ivejp mentioned this pull request Aug 15, 2024

chore(migration): set default value for triggered_at column #1194

Merged

kakcy added 8 commits August 15, 2024 15:50

chore: delete triggeredAt

9dac685

test: fix test

e6c4d79

test: fix test

25a8c25

chore: add AutoOpsRule.IsCompleted

80b30bb

chore: reformat

433675f

chore: reformat

a03d6c7

chore: revert IsCompleted

49c1e65

chore: make proto-all

ab36dfa

kakcy force-pushed the delete-triggered-at-for-auto-ops branch from 7eb8ba5 to ab36dfa Compare August 15, 2024 07:19

kakcy added 3 commits August 15, 2024 16:48

chore: revert value.yaml

06331d5

chore: delete triggered_at column

7fe1124

chore: revert value.yaml

c541c1b

This was referenced Aug 15, 2024

chore: delete triggeredAt for auto_ops_role table #1195

Closed

chore: delete triggeredAt for auto_ops_role table #1196

Merged

cre8ivejp approved these changes Aug 16, 2024

View reviewed changes

kakcy merged commit 3df9296 into main Aug 16, 2024
22 checks passed

kakcy deleted the delete-triggered-at-for-auto-ops branch August 16, 2024 03:03

bucketeer-bot mentioned this pull request Aug 16, 2024

chore(main): release 0.4.7 #1154

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: delete triggeredAt for AutoOps #1179

chore: delete triggeredAt for AutoOps #1179

kakcy commented Aug 1, 2024

kentakozuka left a comment

kentakozuka Aug 5, 2024

kentakozuka Aug 5, 2024

kentakozuka Aug 5, 2024

kentakozuka Aug 5, 2024

cre8ivejp Aug 5, 2024

kakcy Aug 5, 2024

cre8ivejp Aug 5, 2024

kakcy Aug 6, 2024

kakcy Aug 6, 2024

kakcy left a comment

kakcy Aug 5, 2024

cre8ivejp Aug 5, 2024

cre8ivejp Aug 5, 2024

cre8ivejp Aug 5, 2024

cre8ivejp Aug 5, 2024

kakcy Aug 5, 2024

cre8ivejp Aug 5, 2024

kakcy left a comment

kakcy Aug 6, 2024

kakcy Aug 6, 2024

cre8ivejp left a comment

	reserved 4;
	reserved 4; // int64 triggered_at = 4 [deprecated = true];

	reserved 3;
	reserved 3; // ChangeAutoOpsRuleTriggeredAtCommand
	change_auto_ops_rule_triggered_at_command = 3 [deprecated = true];

	reserved 6;
	reserved 6; // int64 triggered_at = 6 [deprecated = true];

chore: delete triggeredAt for AutoOps #1179

chore: delete triggeredAt for AutoOps #1179

Conversation

kakcy commented Aug 1, 2024

kentakozuka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cre8ivejp left a comment

Choose a reason for hiding this comment