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

feat(cmd): add maintenance info #984

Merged
merged 3 commits into from
Jul 18, 2023
Merged

feat(cmd): add maintenance info #984

merged 3 commits into from
Jul 18, 2023

Conversation

curzolapierre
Copy link
Member

@curzolapierre curzolapierre commented Jul 5, 2023 •
edited
Loading 

➜ scalingo --app pierre-sample-go-martini --addon redis database-maintenance-info toto
 !     An error occurred:
       get the maintenance: get database 'ad-9b971c30-43c7-4f40-a43e-c5f256376b77' maintenance: The maintenance toto was not found

➜ scalingo --app pierre-sample-go-martini --addon redis database-maintenance-info 64abc3a59307d595d4110bd3
+------------+--------------------------+
| ID         | 64abc3a59307d595d4110bd3 |
| Type       | db-node-migration        |
| Started At | 2023/07/17 04:05:20      |
| Ended At   | 2023/07/17 04:05:20      |
| Status     | done                     |
+------------+--------------------------+

➜ scalingo --app pierre-sample-go-martini --addon redis database-maintenance-info 64b509d300deff12855f2cc5
+------------+----------------------------------------------+
| ID         | 64b509d300deff12855f2cc5                     |
| Type       | no-op                                        |
| Started At | Next Maintenance Window: 2023/07/24 04:00:00 |
| Ended At   |                                              |
| Status     | scheduled                                    |
+------------+----------------------------------------------+

Fixes: #969

@curzolapierre curzolapierre self-assigned this Jul 5, 2023
@curzolapierre curzolapierre force-pushed the feat/969/maintenance-info branch 4 times, most recently from bd50190 to 07a990f Compare July 11, 2023 15:43
@SCedricThomas SCedricThomas marked this pull request as ready for review July 13, 2023 08:57
@curzolapierre curzolapierre force-pushed the feat/969/maintenance-info branch 2 times, most recently from 5ca164b to 3e7794e Compare July 13, 2023 13:42
@curzolapierre curzolapierre force-pushed the feat/969/maintenance-info branch 2 times, most recently from a6211cd to da03861 Compare July 13, 2023 13:49
SCedricThomas
SCedricThomas requested changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@SCedricThomas SCedricThomas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a tiny nitpick
As I participated in the dev, should we ask for a second reviewer ?

SCedricThomas
SCedricThomas reviewed Jul 13, 2023
View reviewed changes
db/maintenance/info.go Outdated

nextMaintenanceWindowStartingDate, nextMaintenanceWindowEndingDate := getNextLocalMaintenanceWindow(time.Now(), database.MaintenanceWindow, maintenanceInfo.Status)

// we are in the range
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is misplaced right ?

yohann-bacha
yohann-bacha suggested changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@yohann-bacha yohann-bacha left a comment •
edited by curzolapierre
Loading

Choose a reason for hiding this comment

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

it could be a parameterized test here, the test methods look really the same so it's hard to differentiate what they do

db/maintenance/info_test.go
@@ -0,0 +1,194 @@
package maintenance
Copy link
Contributor

@yohann-bacha yohann-bacha Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

suggestion: make a parameterized test here

Copy link
Contributor

@SCedricThomas SCedricThomas Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the test names with the nested structure is a good representation enough, but having a test table can make the tests a bit more clearer

SCedricThomas
SCedricThomas approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@SCedricThomas SCedricThomas left a comment

Choose a reason for hiding this comment

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

LGTM

yohann-bacha
yohann-bacha suggested changes Jul 17, 2023
View reviewed changes
db/maintenance/info_test.go Outdated
"when the maintenance window is ongoing": {
maintenanceTimeData: dateOfTheWeekUTC(time.Wednesday, 7),
expectedMaintenanceRunTime: func(maintenanceData time.Time, maintenanceType scalingo.MaintenanceStatus) time.Time {
if maintenanceType == scalingo.MaintenanceStatusScheduled {
Copy link
Contributor

Choose a reason for hiding this comment

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

question (non-blocking): do you really need to put conditions on your test setup? it makes the test a bit harder to read and understand

Copy link
Member Author

Choose a reason for hiding this comment

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

Without adding more complexity (which wouldn't help for readability) I don't see what would be better, would you have a suggestion for this?

db/maintenance/info_test.go Outdated Show resolved Hide resolved
@curzolapierre curzolapierre force-pushed the feat/969/maintenance-info branch 4 times, most recently from a95f584 to a58b9a2 Compare July 18, 2023 08:15
yohann-bacha
yohann-bacha approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@yohann-bacha yohann-bacha left a comment

Choose a reason for hiding this comment

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

after removing the "expected" field when it's not actually expected, LGTM!

@curzolapierre curzolapierre merged commit 4e89643 into master Jul 18, 2023
@curzolapierre curzolapierre deleted the feat/969/maintenance-info branch July 18, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yohann-bacha yohann-bacha yohann-bacha approved these changes

@SCedricThomas SCedricThomas SCedricThomas approved these changes

Assignees

@curzolapierre curzolapierre

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Maintenance show
3 participants
@curzolapierre @SCedricThomas @yohann-bacha