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
Add Gitlab Connector #1869
Add Gitlab Connector #1869
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1869 +/- ##
==========================================
+ Coverage 99.33% 99.36% +0.03%
==========================================
Files 80 83 +3
Lines 4834 5079 +245
==========================================
+ Hits 4802 5047 +245
Misses 32 32
Continue to review full report at Codecov.
|
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.
This connector looks awesome and this PR is very well written (I wouldn't expect any less from @FabioRosado though!).
I would really like to see an example skill in the docs page for this connector to help folks understand how they can use it. Being able to receive events but not respond would be useful for some scenarios. For example if you configured both the Slack and GitLab connectors and set Slack as the default you could build a simple notification skill.
from opsdroid.matchers import match_event
from opsdroid.connector.gitlab import MRCreated
from opsdroid.events import Message
from opsdroid.skill import Skill
class GitLabNotifier(Skill):
@match_event(MRCreated)
async def notify_mr(self, event):
# Send message to Slack
await self.opsdroid.send(
Message(
text=f"{event.user} just opened the MR '{event.title}' on {event.project}, can someone review it?",
target="#gitlab-notifications"
)
)
I'm on the fence about merging this without any way to interact back to GitLab though. I have no idea what GitLab's API looks like though so have no idea how hard this would be.
I would love to see a simple example skill like this:
from opsdroid.matchers import match_event
from opsdroid.connector.gitlab import MRCreated
from opsdroid.skill import Skill
class GitLabAutoresponder(Skill):
@match_event(MRCreated)
async def respond_mr(self, event):
await event.respond("Welcome to Foo Project, thanks for your merge request! We aim to review new MRs within a week.")
Thank you for the comments and yeah that makes sense, I haven't look much into the rest API but since the baby is starting to sleep 4 hours straight I should be able to keep working on this 😄 I'll address them in the next commits 👍 |
The tests for sending message were mocked because how I decided to build the URL (using the gitlab dataclass) was making it pretty hard to use the new testing method. But seems to be working fine 😄 |
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.
Thanks @FabioRosado this looks really awesome! I don't have a GitLab instance to test against so given that CI is passing I'm happy with this.
Let's merge and if there are any other changes we can follow up with more PRs.
Description
The Gitlab connector is a bit diferent than the other connectors, in this connector we are only getting the payload and trigger events. I haven't implemented a way to reply to events within the connector by using the Message event, since we would need to query the API to do this. I'm thinking that perhaps if you really want to send an automated message you can do it in your skill?
Obviously this can easily be implemented, but I've been working on this connector for one hour in the late nights, not sure how much I can dedicate to it with the new baby 😄
Also, I decided to implement a dataclass to handle the payload because it seems way cleaner to handle the different payload structure from gitlab (it would be awesome if all the payloads would have the same structure but yeah 🤷 )
Finally, I've added the bottom margin for the description lists in our CSS because the events section was all squashed together and it makes it hard to figure out what that blob of text means 😆
Fixes #1861
Status
READY
Type of change
How Has This Been Tested?
Checklist: