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.

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

Jump to bottom

Mattermost support #791

Merged
merged 7 commits into from
Nov 11, 2022
Merged

Mattermost support #791

merged 7 commits into from
Nov 11, 2022

Conversation

Dimedrolity
Copy link
Contributor

Mattermost is an Open Source chat. Need to add Mattermost as delivery channel for Moira.

@Dimedrolity Dimedrolity changed the title Added Mattermost support Mattermost support Oct 20, 2022
kissken
kissken reviewed Oct 20, 2022
View reviewed changes
senders/mattermost/client.go
import "github.com/mattermost/mattermost-server/v6/model"

// Client is wrapper over model.Client4.
type Client interface {
Copy link
Member

Choose a reason for hiding this comment

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

lose interface naming :(

Copy link
Member

Choose a reason for hiding this comment

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

why single file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lose interface naming :(

Read it with package name - mattermost.Client, sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why single file?

I think it is ok to separate types by files, because mattermost.go is about Sender impl. Maybe rename mattermost.go to sender.go 🤔 Think it would be better

Copy link
Member

Choose a reason for hiding this comment

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

interfaces in go ended on "er"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not necessary. The most popular interface in Go is error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed file, I think it is better 8f19954

Copy link
Member

Choose a reason for hiding this comment

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

  • Why separate file?

and about naming Interface names

kissken
kissken reviewed Oct 20, 2022
View reviewed changes
senders/mattermost/mattermost.go Outdated
}
sender.client.SetToken(token)

sender.frontURI = senderSettings["front_uri"]
Copy link
Member

Choose a reason for hiding this comment

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

if uri will empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then trigger URI will be incorrect. I will fix it, thanks

Copy link
Member

Choose a reason for hiding this comment

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

may better validate all struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed acd04f9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may better validate all struct

Now it is ok

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2022 •
edited
Loading

Codecov Report

Merging #791 (8bf1203) into master (426a179) will increase coverage by 0.21%.
The diff coverage is 90.38%.

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
+ Coverage   70.96%   71.18%   +0.21%     
==========================================
  Files         178      179       +1     
  Lines        9237     9341     +104     
==========================================
+ Hits         6555     6649      +94     
- Misses       2305     2311       +6     
- Partials      377      381       +4
Impacted Files Coverage Δ
notifier/registrator.go 37.14% <0.00%> (-1.10%) ⬇️
senders/mattermost/sender.go 92.15% <92.15%> (ø)

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

@Dimedrolity Dimedrolity force-pushed the feature/mattermost branch 2 times, most recently from 601bc34 to 416a8c7 Compare October 25, 2022 05:10
kissken
kissken reviewed Oct 26, 2022
View reviewed changes
go.mod Show resolved Hide resolved
kissken
kissken reviewed Oct 26, 2022
View reviewed changes
senders/mattermost/sender_test.go Outdated
})
}

//TestSender is integration test, run it manually with your Url, Token and Channel ID.
Copy link
Member

Choose a reason for hiding this comment

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

? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is small integration test, you should run it manually. I comment it because it works only with right credentials (your Url, Token and Channel ID). You can run https://hub.docker.com/r/mattermost/mattermost-preview locally, create bot, uncomment test and run it.

@kissken
Copy link
Member

kissken commented Oct 26, 2022

we have new interface, but haven't got any new mock, why? :)

@Dimedrolity
Copy link
Contributor Author

we have new interface, but haven't got any new mock, why? :)

@kissken I created stubs manually without gomock. Do you think that it is better to use gomock?

Now we have 93bba9a#diff-8dd560d964d10bfee0e300f244193cc7a80a8c1dc267f3cba2f393c1051e4c28R26 and 93bba9a#diff-8dd560d964d10bfee0e300f244193cc7a80a8c1dc267f3cba2f393c1051e4c28R43-R49

With gomock

    ctrl := gomock.NewController(t)
    client := mock.NewMockClient(ctrl)
    client.EXPECT().CreatePost(gomock.Any()).Return(nil, nil, errors.New(""))
    sender.client = client

@Dimedrolity Dimedrolity force-pushed the feature/mattermost branch 7 times, most recently from 52affd0 to 8bf1203 Compare November 2, 2022 05:55
@Dimedrolity Dimedrolity marked this pull request as ready for review November 2, 2022 05:59
@Dimedrolity Dimedrolity requested a review from a team as a code owner November 2, 2022 05:59
kissken
kissken previously approved these changes Nov 8, 2022
View reviewed changes
Tetrergeru
Tetrergeru previously approved these changes Nov 8, 2022
View reviewed changes
@Dimedrolity
Copy link
Contributor Author

@kissken Would you please merge it. I suggest to merge without squash, because some commits have important description

@Tetrergeru Tetrergeru dismissed stale reviews from kissken and themself via 7bc3f99 November 10, 2022 10:37
@Dimedrolity
feat(notifier): Added Mattermost sender
4040ca3 
Mattermost is an Open Source chat. Need to add Mattermost as delivery channel for Moira. Added new package for Sender that use mattermost-server dependency to send messages to Moira users via Bot.
@Dimedrolity
refactor(mock): Fix warnings of generate_mocks.sh
3de719b 
There were warnings:
```
#..\go\pkg\mod\github.com\golang\[email protected]\mockgen\mockgen.go:41:2: missing go.sum entry for module providing package golang.org/x/mod/modfile (imported by github.com/golang/mock/mockgen); to add:
#        go get github.com/golang/mock/[email protected]
#..\go\pkg\mod\github.com\golang\[email protected]\mockgen\mockgen.go:42:2: missing go.sum entry for module providing package golang.org/x/tools/imports (imported by github.com/golang/mock/mockgen); to add:
#        go get github.com/golang/mock/[email protected]
```

Use version for mockgen, it is recommended way to install mockgen. Remove `go get -u` because it has no sense, `github.com/golang/mock` is in go.mod.
@Dimedrolity
refactor: Use Gomock
cc2093a 
Use generated mocks instead of manually created.
@Dimedrolity
test: Added manual integration test
b2d2c7d 
There is a case when developer want to check integration with Mattermost locally. Added test for this case. There is build tag "manual" because this test need to run manually (not on common run, not on CI). Nice post about build tags - https://mickey.dev/posts/go-build-tags-testing/
@Dimedrolity
refactor(test): Name magic numbers
871e37d
@Dimedrolity
feat: Add insecure TLS flag
c098339 
Secure connection is important for production env, but is not for dev env.
@Dimedrolity
test: Tests with non-existent keys
d352db3
@Dimedrolity
Copy link
Contributor Author

@kissken Would you please merge it. I suggest to merge without squash, because some commits have important description

Lets try again :)

@kissken kissken merged commit 49176e7 into master Nov 11, 2022
@kissken kissken deleted the feature/mattermost branch November 11, 2022 09:48
@Dimedrolity Dimedrolity mentioned this pull request Dec 7, 2022
Merged
@Dimedrolity Dimedrolity self-assigned this Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tetrergeru Tetrergeru Tetrergeru approved these changes

@kissken kissken kissken approved these changes

Assignees

@Dimedrolity Dimedrolity

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Dimedrolity @codecov-commenter @kissken @Tetrergeru