-
Notifications
You must be signed in to change notification settings - Fork 220
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
feat(service): Add Gotify #487
Conversation
doing this to silence the linter
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.
Hi @heyztb, we appreciate your contribution! Can't wait to get your changes into Notify.
Overall I'm happy with your PR, there's just some minor oversights I'd like you to fix. Also, please add a doc.go
(example) and a README.md
(example).
We're basically ready to merge as soon as you get these fixed! Thank you so much for your efforts.
|
||
func New(appToken, baseURL string) *Gotify { |
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.
Missing comment.
type newMessageRequestBody struct { | ||
Title string | ||
Message string | ||
Priority int | ||
} |
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.
newMessageRequestBody
sounds more like a function, let's simplify it a bit.
type newMessageRequestBody struct { | |
Title string | |
Message string | |
Priority int | |
} | |
type messageRequestBody struct { | |
Title string | |
Message string | |
Priority int | |
} |
|
||
func (g *Gotify) Send(ctx context.Context, subject, message string) error { |
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.
Missing comment.
req, err := http.NewRequestWithContext(context.Background(), "POST", fmt.Sprintf("%s/message", g.baseURL), bytes.NewReader(body)) | ||
if err != nil { | ||
return err | ||
} |
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.
req, err := http.NewRequestWithContext(context.Background(), "POST", fmt.Sprintf("%s/message", g.baseURL), bytes.NewReader(body)) | |
if err != nil { | |
return err | |
} | |
req, err := http.NewRequestWithContext(ctx, http.MethodPost, fmt.Sprintf("%s/message", g.baseURL), bytes.NewReader(body)) | |
if err != nil { | |
return errors.Wrap(err, "create new request") | |
} |
|
||
body, err := json.Marshal(reqBody) | ||
if err != nil { | ||
return err |
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.
return err | |
return errors.Wrap(err, "encode message body") |
case <-ctx.Done(): | ||
return ctx.Err() | ||
default: | ||
reqBody := &newMessageRequestBody{ |
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.
reqBody := &newMessageRequestBody{ | |
reqBody := &messageRequestBody{ |
req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", g.appToken)) | ||
|
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 know the functionalities of Gotify too well, but I'm pretty sure that we can safely add this to be more up to standards.
req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", g.appToken)) | |
req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", g.appToken)) | |
req.Header.Set("Content-Type", "application/json; charset=utf-8") | |
b, err := g.httpClient.Do(req) | ||
b.Body.Close() | ||
if err != nil { | ||
return errors.Wrapf(err, "failed to send message to gotify 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.
Let's be more careful with the success of our requests, also prevents potential resource leaks.
b, err := g.httpClient.Do(req) | |
b.Body.Close() | |
if err != nil { | |
return errors.Wrapf(err, "failed to send message to gotify server") | |
} | |
resp, err := g.httpClient.Do(req) | |
if err != nil { | |
return errors.Wrapf(err, "send request to gotify server") | |
} | |
// Read response and verify success | |
result, err := io.ReadAll(resp.Body) | |
if err != nil { | |
return errors.Wrap(err, "read response") | |
} | |
if resp.StatusCode != http.StatusOK { | |
return fmt.Errorf("gotify returned status code %d: %s", resp.StatusCode, string(result)) | |
} |
Hi @nikoksr I'll push changes as per your suggestions once I have a bit of time sometime this week. Thanks! |
Closing due to inactivity. |
Description
Adds support for Gotify.
Motivation and Context
Closes #455
How Has This Been Tested?
Unit tests are included.
Screenshots / Output (if appropriate):
Types of changes
Checklist: