-
Notifications
You must be signed in to change notification settings - Fork 86
CLOUDP-57306: List alerts (Atlas) #60
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
Alerts(string, *atlas.AlertsListOptions) (*atlas.AlertsResponse, error) | ||
} | ||
|
||
type AlertsStore interface { |
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.
[qa] I do not why if I do not create the interface AlertsStore, mongocli atlas alert describe alertID
seems using the wrong interface (it returns a list instead of one alert)
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 appreciate the effort making this work with the filter but let's get this working with what we have right now and let's make a separate PR when we have the status merged, let's not block this unless necessary
internal/cli/atlas_alerts_list.go
Outdated
|
||
cmd.Flags().IntVar(&opts.pageNum, flags.Page, 0, usage.Page) | ||
cmd.Flags().IntVar(&opts.itemsPerPage, flags.Limit, 0, usage.Limit) | ||
cmd.Flags().StringVar(&opts.status, flags.Status, "", usage.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.
Thanks for making this effort but let's make this work without the filter for now and let's create a ticket linking the dependency on mongodb/go-client-mongodb-atlas#69
this let us have something working and we can later add more features
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.
Can I just comment out the piece of code related to the status? or do you want me to delete it
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.
This line in particular can be commented if you want, but I saw that you changed the signature of the API so for that we can't do anything about it 🤷♂
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.
Looks good let's just add a link to the jira where we'll add the missing functionality so it's not lost the reasoning behind the comments
|
||
cmd.Flags().IntVar(&opts.pageNum, flags.Page, 0, usage.Page) | ||
cmd.Flags().IntVar(&opts.itemsPerPage, flags.Limit, 0, usage.Limit) | ||
//cmd.Flags().StringVar(&opts.status, flags.Status, "", usage.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.
can you please add a // TODO: CLOUDP-N
for the ticket where we'll fix this so people know why this is commented and when we plan to fix it?
} | ||
|
||
}) | ||
// t.Run("List with status OPEN", func(t *testing.T) { |
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.
Let's add a TODO
here as well linking to jira
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
(DOCSP-24168) AtlasCLI version 1.1.5
Jira ticket: CLOUDP-57306
Checklist
make fmt
and formatted my codeFurther comments