Skip to content

CLOUDP-62053: It is not possible to unack an alert #160

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

Merged
merged 11 commits into from
May 7, 2020
Merged

CLOUDP-62053: It is not possible to unack an alert #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7c198bc
Refactoring
andreaangiolillo Apr 29, 2020
1dacaa0
Merge remote-tracking branch 'origin/master'
andreaangiolillo Apr 29, 2020
39b30ca
Merge remote-tracking branch 'origin/master'
andreaangiolillo Apr 29, 2020
861d88e
Merge remote-tracking branch 'origin/master'
andreaangiolillo Apr 30, 2020
a1ba80a
Merge remote-tracking branch 'origin/master'
andreaangiolillo May 5, 2020
658d057
Merge remote-tracking branch 'origin/master'
andreaangiolillo May 5, 2020
5927427
Merge remote-tracking branch 'origin/master'
andreaangiolillo May 7, 2020
36b93ed
CLOUDP-62053: It is not possible to unack an alert
andreaangiolillo May 7, 2020
b7c7ca0
Merge remote-tracking branch 'origin/master' into CLOUDP-62053
andreaangiolillo May 7, 2020
81e9a68
Updated comment
andreaangiolillo May 7, 2020
948c928
Merge remote-tracking branch 'origin/master' into CLOUDP-62053
andreaangiolillo May 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions internal/cli/atlas_alerts_acknowledge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type atlasAlertsAcknowledgeOpts struct {
alertID string
until string
comment string
forever bool
store store.AlertAcknowledger
}

Expand All @@ -52,20 +53,18 @@ func (opts *atlasAlertsAcknowledgeOpts) Run() error {
}

func (opts *atlasAlertsAcknowledgeOpts) newAcknowledgeRequest() *atlas.AcknowledgeRequest {
until := opts.until

// To acknowledge an alert “forever”, set the field value to 100 years in the future.
if until == "" {
until = time.Now().AddDate(100, 1, 1).Format(time.RFC3339)
if opts.forever {
opts.until = time.Now().AddDate(100, 1, 1).Format(time.RFC3339)
}

return &atlas.AcknowledgeRequest{
AcknowledgedUntil: until,
AcknowledgedUntil: opts.until,
Copy link
Collaborator

Choose a reason for hiding this comment

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

[q] do we need to update the client? this field has an omit empty so I'm wondering how is it working with blank?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we have to update the atlas client. The field is set to nil when the until flag is black which is what we want here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

AcknowledgedUntil is not a pointer so it can't be nil, only empty ("")
when we serialize it it would only be something like an empty object {} so maybe the API is assuming that since there's no key is null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought that the serialization of an empty string with omitempty was nil since I got null when I printed the value of until in the server.

Copy link
Collaborator

Choose a reason for hiding this comment

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

omitempty means that it won't serialize/deserialize that key not about its content, the content defines the rule for omitempty, which in the case of string is the empty string ""

Copy link
Collaborator Author

@andreaangiolillo andreaangiolillo May 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I noticed it, I was reading this article https://www.sohamkamani.com/golang/2018-07-19-golang-omitempty/ that explains it very well. Regarding your question, as you previously said, the only explanation seems to be that the API sets the object to null when there is no key (which makes sense).

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, omitempty is confusing, I know, is one of the things took me the most to get on go, and we can trip a lot with it
if you are sure this works as expected we can merge but we need to be aware that a change on the API can break this command

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am quite confident that it is working as expected but we can make AcknowledgedUntil in the atlas client a pointer just to be sure that this behaviour is not going to change in the future. Let me know if you want me to open a PR in the altlas go client.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@andreaangiolillo sorry, was busy and forgot about this, up to you, I'm ok to merge this as it is and we can improve the client later

AcknowledgementComment: opts.comment,
}
}

// mongocli atlas alerts acknowledge alertID --projectId projectId
// mongocli atlas alerts acknowledge alertID --projectId projectId --forever
func AtlasAlertsAcknowledgeBuilder() *cobra.Command {
opts := new(atlasAlertsAcknowledgeOpts)
cmd := &cobra.Command{
Expand All @@ -82,6 +81,7 @@ func AtlasAlertsAcknowledgeBuilder() *cobra.Command {
},
}

cmd.Flags().BoolVarP(&opts.forever, flags.Forever, flags.ForeverShort, false, usage.Forever)
cmd.Flags().StringVar(&opts.until, flags.Until, "", usage.Until)
cmd.Flags().StringVar(&opts.comment, flags.Comment, "", usage.Comment)

Expand Down
2 changes: 2 additions & 0 deletions internal/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ const (
Members = "members" // Members flag
MembersShort = "m" // MembersShort flag
Tier = "tier" // Tier flag
Forever = "forever" // Forever flag
ForeverShort = "F" // ForeverShort flag
DiskSizeGB = "diskSizeGB" // DiskSizeGB flag
MDBVersion = "mdbVersion" // MDBVersion flag
Backup = "backup" // Backup flag
Expand Down
1 change: 1 addition & 0 deletions internal/usage/usage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const (
AuthDB = "Authentication database name."
Granularity = "Duration in ISO 8601 notation that specifies the interval between measurement data points."
Page = "Page number."
Forever = "Acknowledge an alert “forever”."
Status = "Alert's status."
Until = "Acknowledged until a date."
Limit = "Number of items per page."
Expand Down