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

feat: Add MatterMost remote #508

Merged
merged 8 commits into from
Aug 8, 2024
Merged

Conversation

JamesMConroy
Copy link
Contributor

@JamesMConroy JamesMConroy commented May 3, 2024

Proposed change

Adds a basic mattermost remote to handle receiving webhook events and posting messages with the API.

Should resolve #97

Types of changes

What types of changes is this pull request introducing to flottbot? Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

You can fill this out after creating your PR. Put an x in the boxes that apply

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Testing

To test I used the docker image and add user script from the MM bot example repo https://github.com/mattermost-community/mattermost-bot-sample-golang

Adds a basic mattermost remote to handle recieving webhook events and
posting messages with the API.
@wass3rw3rk
Copy link
Member

thanks for the PR and apologies for the delay, i will give it a look in the coming days. also, looking to test it on a real or fake mattermost instance

@JamesMConroy
Copy link
Contributor Author

No worries @wass3rw3rk stuff happens and I've had PRs sit idle for longer.

This did work with that local mattermost development container, but it didn't work with my company's MatterMost.

We use the version packaged with Gitlab Omnibus and it's a pain to test against because it is in a sandbox environment.

I don't know if it is our configuration or an issue with how I'm using the websocket library, but I would get a panic when trying to connect.

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

I got a local version of Mattermost stood up with Docker and things worked pretty well. A couple of notes below.

Thanks again for the patience.

remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
Also added a variable to set the protocol to insecure (http, wss)
@JamesMConroy
Copy link
Contributor Author

@wass3rw3rk Just tested these changes on my work (instance with ssl) and it's working 🎉

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

thanks for testing and tweaks. one more question/suggestion and i think we're good.

models/bot.go Show resolved Hide resolved
Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

hey, promise this is the last round from me 😅 might seem like a lot, but most of it is just lower casing log messages to be consistent with the other log messages.

thanks for your patience and fixups!

after those are fixed, we'll run github actions workflows on it and there might be a couple of style tweaks to follow up with 🤞🏼

core/configure.go Outdated Show resolved Hide resolved
core/configure.go Outdated Show resolved Hide resolved
core/configure.go Outdated Show resolved Hide resolved
core/remotes.go Outdated Show resolved Hide resolved
core/remotes.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
models/bot.go Show resolved Hide resolved
JamesMConroy and others added 3 commits July 16, 2024 10:44
Made the added log messages lowercase to match the style of the other log messages.

Also corrected some spelling mistakes

Co-authored-by: David May <49894298+wass3rw3rk@users.noreply.github.com>
As suggested in code review.

Co-authored-by: David May <49894298+wass3rw3rk@users.noreply.github.com>
Removed an else branch as suggested in code review

Co-authored-by: David May <49894298+wass3rw3rk@users.noreply.github.com>
@JamesMConroy
Copy link
Contributor Author

@wass3rw3rk no worries, I like making sure things are consistent.

This is the first time I've worked with golangci-lint so I'm sure there are some style issues I've missed.

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

i think this last change should clear out the errors. thanks for the perseverance.

remote/mattermost/remote.go Outdated Show resolved Hide resolved
James Conroy added 3 commits July 26, 2024 18:37
Also removed some useless newlines.
Just to make golangci-lint happy.
It was never being used so it caused the linter to fail
Now the all the variables it assigns are actually assigned so the binary
will build
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10118959198

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 8806816327: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

tried this again with local set up and works good! defining insecure as "1" to enable it tripped me up a little. no big deal though as long as we capture in docs.

thanks again (also for the patience)

@wass3rw3rk wass3rw3rk merged commit f2a2de6 into target:main Aug 8, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Mattermost as a remote?
3 participants