-
Notifications
You must be signed in to change notification settings - Fork 83
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
CLOUDP-62053: It is not possible to unack an alert #160
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.
Fix looks good to me I just have a question and is more out of curiosity than anything else
} | ||
|
||
return &atlas.AcknowledgeRequest{ | ||
AcknowledgedUntil: until, | ||
AcknowledgedUntil: opts.until, |
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] do we need to update the client? this field has an omit empty so I'm wondering how is it working with blank?
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 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.
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.
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?
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 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.
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.
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 ""
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.
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).
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.
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
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 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.
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.
@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
Proposed changes
Jira ticket: CLOUDP-62053
Checklist
make fmt
and formatted my codeFurther comments
ACK:
ACK FOREVER:
UNACK: