Skip to content
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

Refactor Senders initialization #804

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Dimedrolity
Copy link
Contributor

@Dimedrolity Dimedrolity commented Nov 11, 2022

Closes #797

@Dimedrolity Dimedrolity changed the title refactor: Issue https://github.com/moira-alert/moira/issues/797 Refactor Senders initialization Nov 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2022

Codecov Report

Merging #804 (c6e66a5) into master (0e97cc7) will decrease coverage by 0.01%.
The diff coverage is 62.39%.

@@            Coverage Diff             @@
##           master     #804      +/-   ##
==========================================
- Coverage   71.32%   71.30%   -0.02%     
==========================================
  Files         179      179              
  Lines        9322     9348      +26     
==========================================
+ Hits         6649     6666      +17     
- Misses       2293     2303      +10     
+ Partials      380      379       -1     
Impacted Files Coverage Δ
notifier/registrator.go 33.78% <0.00%> (-3.36%) ⬇️
notifier/senders/calc_message_parts.go 100.00% <ø> (ø)
notifier/senders/discord/response.go 80.00% <ø> (ø)
notifier/senders/discord/send.go 68.23% <ø> (ø)
notifier/senders/mail/mail.go 44.82% <0.00%> (ø)
notifier/senders/mail/send.go 71.66% <ø> (ø)
notifier/senders/opsgenie/send.go 77.90% <ø> (ø)
notifier/senders/pagerduty/send.go 74.62% <ø> (ø)
notifier/senders/read_image_store_config.go 100.00% <ø> (ø)
notifier/senders/telegram/handle_message.go 72.00% <ø> (ø)
... and 21 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Dimedrolity Dimedrolity force-pushed the feature/sender-constructor branch 3 times, most recently from b56416b to 82fdc94 Compare November 14, 2022 09:12
@Dimedrolity
Copy link
Contributor Author

Want to move senders directory to notifier directory, because senders have no sense without notifier. Also errors.go in the root of Moira contains Notifier erros, should be also moved to notifier dir.

@Dimedrolity
Copy link
Contributor Author

@Dimedrolity Dimedrolity marked this pull request as ready for review November 22, 2022 08:39
@Dimedrolity Dimedrolity requested a review from a team as a code owner November 22, 2022 08:39
Copy link
Member

@kissken kissken left a comment

Choose a reason for hiding this comment

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

left some comments

notifier/registrator.go Show resolved Hide resolved
notifier/registrator.go Outdated Show resolved Hide resolved
notifier/registrator.go Show resolved Hide resolved
notifier/senders/discord/sender.go Show resolved Hide resolved
notifier/senders/discord/sender.go Outdated Show resolved Hide resolved
notifier/senders/discord/sender.go Outdated Show resolved Hide resolved
sender.logger = logger
sender.location = location
sender.frontURI = senderSettings["front_uri"]
maxEvents, err := strconv.Atoi(senderSettings["max_events"])
if err != nil {
return fmt.Errorf("max_events should be an integer: %w", err)
return nil, fmt.Errorf("max_events should be an integer: %w", err)
}
sender.maxEvents = maxEvents
sender.client = &http.Client{
Timeout: time.Duration(30) * time.Second, //nolint
Copy link
Member

Choose a reason for hiding this comment

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

let's move it into const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done db17931

Copy link
Member

Choose a reason for hiding this comment

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

not in func, please :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just drop //nolint, it is not necessary to refactor all Moira code. This const timeout is so little thing compare to problems that we have in Moira, so I advise you don't pay attention to it.

notifier/senders/script/script.go Outdated Show resolved Hide resolved
notifier/senders/selfstate/selfstate.go Outdated Show resolved Hide resolved
notifier/senders/webhook/webhook_test.go Outdated Show resolved Hide resolved
@Dimedrolity Dimedrolity requested a review from a team November 28, 2022 07:41
Tetrergeru
Tetrergeru previously approved these changes Dec 5, 2022
Use constructor function NewSender instead of Init method, remove Init method from Sender interface.
Rename `init.go` files to `sender.go`, because files no more contain Init method.
Moved senders directory to notifier directory, because senders have no sense without notifier, senders is a part of notifier. Also resolve some warnings
@Dimedrolity Dimedrolity requested review from kissken and removed request for kissken December 7, 2022 05:03
@kissken
Copy link
Member

kissken commented Dec 7, 2022

merge after release

@kissken
Copy link
Member

kissken commented Dec 9, 2022

and let's make pretty commit messages

@Dimedrolity
Copy link
Contributor Author

We don't need pretty, we need self descriptive. These messages are self descriptive enough. It will be squashed in one commit on merge anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Senders initialization
4 participants