-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
Add maintenance listing #982
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the review when the PR is only in draft, but I thought of it so here it is.
Waiting for go-scalingo release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nitpicking, otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please rebase on master?
I've suggested the change you probably need to do after the rebase.
Due to this PR: #983
cmd/maintenance.go
Outdated
PerPage: c.Int("per-page"), | ||
}) | ||
if err != nil { | ||
errorQuitWithHelpMessage(err, c, "database-maintenance-list") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we want to show help message here.
E.g. in case of error when calling DatabaseListMaintenance
, just print the error message should be enough
Co-authored-by: Pierre Curzola <[email protected]>
Co-authored-by: Pierre Curzola <[email protected]>
Co-authored-by: Pierre Curzola <[email protected]>
Co-authored-by: Pierre Curzola <[email protected]>
Co-authored-by: Pierre Curzola <[email protected]>
Co-authored-by: Pierre Curzola <[email protected]>
8b60c1a
to
3dd4cb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nitpicks otherwise LGTM
db/maintenance/list.go
Outdated
) | ||
|
||
func List(ctx context.Context, app string, addonName string, paginationOpts scalingo.PaginationOpts) error { | ||
caser := cases.Title(language.English) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity why are you using cases
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because strings.Title is deprecated: https://stackoverflow.com/questions/71620717/in-go-1-18-strings-title-is-deprecated-what-to-use-now-and-how
db/maintenance/list.go
Outdated
caser := cases.Title(language.English) | ||
c, err := config.ScalingoClient(ctx) | ||
if err != nil { | ||
return errgo.Notef(err, "get Scalingo client") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use Wrap
now, according to our doc
db/maintenance/list.go
Outdated
|
||
response, err := c.DatabaseListMaintenance(ctx, app, addonName, paginationOpts) | ||
if err != nil { | ||
return errgo.Notef(err, "list the database maintenance") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
return errgo.Notef(err, "list the database maintenance") | |
return errgo.Wrap(err, "list the database maintenance") |
db/maintenance/list.go
Outdated
caser.String(string(maintenance.Type)), | ||
startedAt, | ||
endedAt, | ||
caser.String(string(maintenance.Status)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know the Title
method, thanks for sharing 👍
But I don't think that we want to format these fields (i.e. Type
, Status
). Becuase IMO they should be formatted like they are in platform side in case of issue in user side. That would simplify our investigation, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I respected the user story, but you may be right here: https://github.com/Scalingo/project-items/issues/285
fix #968