-
Notifications
You must be signed in to change notification settings - Fork 86
CLOUDP-57846: List global alerts in mongocli #67
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
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.
One small question and one small change but looks great
} | ||
|
||
func (opts *opsManagerAlertsGlobalListOpts) Run() error { | ||
alertOpts := om.AlertsListOptions{ |
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.
[q] what about pagination?
[nit] you could also do &om.AlertsListOptions{
here and not do the &
on line 42
internal/store/global_alerts.go
Outdated
// GlobalAlerts encapsulate the logic to manage different cloud providers | ||
func (s *Store) GlobalAlerts(opts *om.AlertsListOptions) (*om.GlobalAlerts, error) { | ||
switch s.service { | ||
case config.OpsManagerService, config.CloudManagerService: |
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 think this is config.OpsManagerService
only so you may need to remove config.CloudManagerService
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.
LGTM
func (opts *opsManagerAlertsGlobalListOpts) Run() error { | ||
alertOpts := &om.AlertsListOptions{ | ||
Status: opts.status, | ||
ListOptions: mongodbatlas.ListOptions{ |
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 as FYI (no need for any change here) you can also do
alertOpts := &om.AlertsListOptions{}
alertOpts.PageNum = opts.pageNum
go has automatic delegation for embedded structs, this is just style and there's no advantage of one over the other, so just letting you know
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.
Also for future work, we should do with pagination what we did with delete that we embed a struct with the common things that all paginated resources do
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 will create the activity on Jira
DOCSP-24941 Add support for Chocholatey to the installation options
Jira ticket: CLOUDP-57846
Checklist
make fmt
and formatted my codeFurther comments
./bin/mongocli om alerts global list --profile OPS