-
-
Notifications
You must be signed in to change notification settings - Fork 633
sfe: Periodically import approved rate limit override tickets #8372
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
0a40f0d to
d149fd6
Compare
d149fd6 to
504a301
Compare
e89aef5 to
32d6381
Compare
32d6381 to
5233b10
Compare
| // - "even": process only tickets with even IDs | ||
| // - "odd": process only tickets with odd IDs | ||
| // If unspecified or empty, defaults to "all". | ||
| Mode string `validate:"omitempty,required_with=Interval,oneof=all even odd"` |
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.
Nit: Implementing this as two "M of N" settings (with the same underlying modulo bucketing logic) is pretty simple and would allow a lot more flexibility. But this all/even/odd mode is already implemented and will work fine for us, so NBD.
jsha
left a comment
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.
Generally looks great! A few small tweaks, and one proposed refactoring for processTicket.
| "Once the error has been corrected, change the status back to \"solved\" to retry.\n", | ||
| cause, | ||
| ) | ||
| err := im.zendesk.UpdateTicketStatus(ticketID, "pending", privateBody, false) |
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.
As a possible followup: I'd love to split UpdateTicketStatus into UpdateTicketPublic and UpdateTicketPrivate, since boolean fields can make it easy to make copy-paste mistakes. Definitely not required as part of this PR.
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 like that idea. I had to write tests to make sure that we got the intended result and it should keyed me in to the idea that these two should be a little more separate.
|
@beautifulentropy, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values. |
Add a periodic background job to the SFE with the following responsibilities:
Closes #8166