Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ fedmsg
fedora-messaging
PyGithub
pypandoc_binary
python-gitlab
urllib3
jinja2
flask
Expand Down
39 changes: 39 additions & 0 deletions sync2jira/api/gitlab_client.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
from gitlab import Gitlab


class GitlabClient:

def __init__(self, url, token, project):
self.url = url
self.token = token
self.project = project
self._client = Gitlab(url=url, private_token=token)
self._project = self._client.get(self.project)

def fetch_issue(self, iid):
return self._project.issues.get(iid)
Comment on lines +13 to +14
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good if you include type hints in the function signatures. E.g.,

Suggested change
def fetch_issue(self, iid):
return self._project.issues.get(iid)
def fetch_issue(self, iid: str) -> gitlab.v4.objects.ProjectIssueManager:
return self._project.issues.get(iid)

You can omit the type for self, but it would be good if all the other parameters as well as any return value were typed. (I had to guess at the types in my example, here....)


def fetch_notes_for_issue(self, iid):
issue = self.fetch_issue(iid)
return GitlabClient.map_notes_to_intermediary(issue.notes.list(all=True))
Comment on lines +16 to +18
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even though it's a static method, you can still refer to map_notes_to_intermediary() using self, and doing so avoids having to name the class within its implementation (which is awkward, if someone decides to rename the class or create a subclass of it).

Suggested change
def fetch_notes_for_issue(self, iid):
issue = self.fetch_issue(iid)
return GitlabClient.map_notes_to_intermediary(issue.notes.list(all=True))
def fetch_notes_for_issue(self, iid):
issue = self.fetch_issue(iid)
return self.map_notes_to_intermediary(issue.notes.list(all=True))

Ditto for fetch_notes_for_mr().

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Alternatively, you can use __class__ instead of self, but I find that to be kind of ugly.)


def fetch_mr(self, iid):
return self._project.mergerequests.get(iid)

def fetch_notes_for_mr(self, iid):
mr = self.fetch_mr(iid)
return GitlabClient.map_notes_to_intermediary(mr.notes.list(all=True))

@staticmethod
def map_notes_to_intermediary(notes):
return [
{
"author": note.author.username,
"name": note.author.name,
"body": note.body,
"id": note.id,
"date_created": note.created_at,
"changed": note.updated_at,
}
for note in notes
]
23 changes: 23 additions & 0 deletions sync2jira/handler/base.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import logging

# Local Modules
import sync2jira.handler.github as gh
import sync2jira.handler.gitlab as gl

log = logging.getLogger("sync2jira")


def get_handler_for(suffix, topic, idx):
"""
Function to check if a handler for given suffix is configured
:param String suffix: Incoming suffix
:param String topic: Topic of incoming message
:param String idx: Id of incoming message
:returns: Handler function if configured for suffix. Otherwise None.
"""
if suffix.startswith("github"):
return gh.get_handler_for(suffix, topic, idx)
elif suffix.startswith("gitlab"):
return gl.get_handler_for(suffix, topic, idx)
log.info("Unsupported datasource %r", suffix)
return None
110 changes: 110 additions & 0 deletions sync2jira/handler/github.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import logging

# Local Modules
import sync2jira.downstream_issue as d_issue
import sync2jira.downstream_pr as d_pr
import sync2jira.handler.github_upstream_issue as u_issue
import sync2jira.handler.github_upstream_pr as u_pr
from sync2jira.intermediary import matcher

log = logging.getLogger("sync2jira")


def handle_issue_msg(body, headers, suffix, config):
"""
Function to handle incoming github issue message
:param Dict body: Incoming message body
:param Dict headers: Incoming message headers
:param String suffix: Incoming suffix
:param Dict config: Config dict
"""
# GitHub '.issue*' is used for both PR and Issue; check if this update
# is actually for a PR
if "pull_request" in body["issue"]:
if body["action"] == "deleted":
# I think this gets triggered when someone deletes a comment
# from a PR. Since we don't capture PR comments (only Issue
# comments), we don't need to react if one is deleted.
log.debug("Not handling PR 'action' == 'deleted'")
return
# Handle this PR update as though it were an Issue, if that's
# acceptable to the configuration.
if not (pr := u_issue.handle_github_message(body, config, is_pr=True)):
log.info("Not handling PR issue update -- not configured")
return
# PRs require additional handling (Issues do not have suffix, and
# reporter needs to be reformatted).
pr.suffix = suffix
pr.reporter = pr.reporter.get("fullname")
setattr(pr, "match", matcher(pr.content, pr.comments))
d_pr.sync_with_jira(pr, config)
else:
if issue := u_issue.handle_github_message(body, config):
d_issue.sync_with_jira(issue, config)
else:
log.info("Not handling Issue update -- not configured")


def handle_pr_msg(body, headers, suffix, config):
"""
Function to handle incoming github PR message
:param Dict body: Incoming message body
:param Dict headers: Incoming message headers
:param String suffix: Incoming suffix
:param Dict config: Config dict
"""
if pr := u_pr.handle_github_message(body, config, suffix):
d_pr.sync_with_jira(pr, config)
else:
log.info("Not handling PR update -- not configured")


# Issue related handlers
issue_handlers = {
# GitHub
# New webhook-2fm topics
"github.issues": handle_issue_msg,
"github.issue_comment": handle_issue_msg,
# Old github2fedmsg topics
"github.issue.opened": handle_issue_msg,
"github.issue.reopened": handle_issue_msg,
"github.issue.labeled": handle_issue_msg,
"github.issue.assigned": handle_issue_msg,
"github.issue.unassigned": handle_issue_msg,
"github.issue.closed": handle_issue_msg,
"github.issue.comment": handle_issue_msg,
"github.issue.unlabeled": handle_issue_msg,
"github.issue.milestoned": handle_issue_msg,
"github.issue.demilestoned": handle_issue_msg,
"github.issue.edited": handle_issue_msg,
}

# PR related handlers
pr_handlers = {
# GitHub
# New webhook-2fm topics
"github.pull_request": handle_pr_msg,
"github.issue_comment": handle_pr_msg,
# Old github2fedmsg topics
"github.pull_request.opened": handle_pr_msg,
"github.pull_request.edited": handle_pr_msg,
"github.issue.comment": handle_pr_msg,
"github.pull_request.reopened": handle_pr_msg,
"github.pull_request.closed": handle_pr_msg,
}
Comment on lines +62 to +94
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the current (proposed) code, I don't see any value in maintaining these as separate tables:

  • There are almost no key collisions
  • We always look in both tables in a prescribed order

So, we can merge the tables and resolve the conflict. That will simplify the GitHub get_handler_for() code, and, in fact, it will offer the opportunity to have the base get_handler_for() implementation selecting the table instead of selecting the function (which reads the table).



def get_handler_for(suffix, topic, idx):
"""
Function to check if a handler for given suffix is configured
:param String suffix: Incoming suffix
:param String topic: Topic of incoming message
:param String idx: Id of incoming message
:returns: Handler function if configured for suffix. Otherwise None.
"""
if suffix in issue_handlers:
return issue_handlers.get(suffix)
elif suffix in pr_handlers:
return pr_handlers.get(suffix)
log.info("No github handler for %r %r %r", suffix, topic, idx)
return None
Comment on lines +105 to +110
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use the power of get() and avoid a redundant lookup:

Suggested change
if suffix in issue_handlers:
return issue_handlers.get(suffix)
elif suffix in pr_handlers:
return pr_handlers.get(suffix)
log.info("No github handler for %r %r %r", suffix, topic, idx)
return None
handler = issue_handlers.get(suffix, pr_handlers.get(suffix))
if not handler:
log.info("No github handler for %r %r %r", suffix, topic, idx)
return handler

Comment on lines +109 to +110
Copy link
Collaborator

@webbnh webbnh Oct 10, 2025

Choose a reason for hiding this comment

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

Under the principle of "separation of policy and mechanism", I think the call to log.info() belongs in the caller of this function rather than here.

For instance, how do we know that the caller would not want to declare a WARNING or ERROR (or perhaps log it only for DEBUG'ing). Also, this function requires only the suffix parameter -- the others are supplied only for logging purposes -- so if we leave the logging to the caller then we can simplify the function interface.

(FWIW, my gut instinct is that, if we cannot find a handler, either that's an "error" or something totally uninteresting...so, INFO is almost certainly the wrong level...and we certainly don't have enough context here to pick the correct level, so we shouldn't be logging this here.)

Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@

from github import Github, UnknownObjectException

import sync2jira.handler.github_upstream_issue as u_issue
import sync2jira.intermediary as i
import sync2jira.upstream_issue as u_issue

log = logging.getLogger("sync2jira")

Expand Down
136 changes: 136 additions & 0 deletions sync2jira/handler/gitlab.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import logging

from sync2jira.api.gitlab_client import GitlabClient
import sync2jira.downstream_issue as d_issue
import sync2jira.downstream_pr as d_pr

# Local Modules
import sync2jira.intermediary as i

log = logging.getLogger("sync2jira")


def should_sync(upstream, labels, config, event_type):
mapped_repos = config["sync2jira"]["map"]["gitlab"]
if upstream not in mapped_repos:
log.debug("%r not in Gitlab map: %r", upstream, mapped_repos.keys())
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be helpful if the mapped repos list were presented in sorted order.

Suggested change
log.debug("%r not in Gitlab map: %r", upstream, mapped_repos.keys())
log.debug("%r not in Gitlab map: %r", upstream, sorted(mapped_repos))

return None
if event_type not in mapped_repos[upstream].get("sync", []):
log.debug(
"%r not in Gitlab sync map: %r",
event_type,
mapped_repos[upstream].get("sync", []),
)
Comment on lines +18 to +23
Copy link
Collaborator

@webbnh webbnh Oct 10, 2025

Choose a reason for hiding this comment

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

The common subexpression is complicated (and long) enough that it's worth creating a local variable for (if for no reason other than to get away from Black's line wrapping and to cut the number of lines in half...):

Suggested change
if event_type not in mapped_repos[upstream].get("sync", []):
log.debug(
"%r not in Gitlab sync map: %r",
event_type,
mapped_repos[upstream].get("sync", []),
)
events = mapped_repos[upstream].get("sync", [])
if event_type not in events:
log.debug("%r not in Gitlab sync map: %r", event_type, sorted(events))

return None

_filter = config["sync2jira"].get("filters", {}).get("gitlab", {}).get(upstream, {})
for key, expected in _filter.items():
if key == "labels":
if labels.isdisjoint(expected):
log.debug("Labels %s not found on issue: %s", expected, upstream)
Comment on lines +29 to +30
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very cool: I hadn't grokked the fact that the argument (expected) didn't have to be another set but could be any iterable. Nice!

return None


def handle_gitlab_issue(body, headers, config, suffix):
"""
Handle GitLab issue from FedMsg.

:param Dict body: FedMsg Message body
:param Dict body: FedMsg Message headers
:param Dict config: Config File
:param Bool is_pr: msg refers to a pull request
"""
upstream = body["project"]["path_with_namespace"]
url = headers["x-gitlab-instance"]
token = config["sync2jira"].get("github_token")
Comment on lines +44 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should that be gitlab_token instead of github_token?

Ditto line 64 et al.

labels = {label["title"] for label in body.get("labels", [])}
iid = body.get("object_attributes").get("iid")

if should_sync(upstream, labels, config, "issue"):
sync_gitlab_issue(GitlabClient(url, token, upstream), iid, upstream, config)


def handle_gitlab_note(body, headers, config, suffix):
"""
Handle Gitlab note from FedMsg.

:param Dict body: FedMsg Message body
:param Dict body: FedMsg Message headers
:param Dict config: Config File
:param String suffix: FedMsg suffix
"""
upstream = body["project"]["path_with_namespace"]
url = headers["x-gitlab-instance"]
token = config["sync2jira"].get("github_token")

if "merge_request" in body:
labels = {
label["title"] for label in body.get("merge_request").get("labels", [])
}
iid = body.get("merge_request").get("iid")
Comment on lines +66 to +70
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the "guard" at line 66, you don't need to use the first get() at lines 68 and 70 (i.e., we could use square brackets). Moreover, we might as well save the cost of the lookup and cache the result in a local variable.

Suggested change
if "merge_request" in body:
labels = {
label["title"] for label in body.get("merge_request").get("labels", [])
}
iid = body.get("merge_request").get("iid")
mr = body.get("merge_request")
if mr:
labels = {label["title"] for label in mr.get("labels", [])}
iid = mr.get("iid")

BTW, I think we'll be OK if labels turns up empty, but what if iid turns out to be None? It seems like either we should trust that "iid" is there (and use square brackets instead of get()) or we should test iid before we call sync*().

Copy link
Collaborator

Choose a reason for hiding this comment

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

An analogous comment applies to lines 74-79 re body.get("issue").


if should_sync(upstream, labels, config, "issue"):
sync_gitlab_mr(GitlabClient(url, token, upstream), iid, upstream)
if "issue" in body:
labels = {label["title"] for label in body.get("issue").get("labels", [])}
iid = body.get("issue").get("iid")

if should_sync(upstream, labels, config, "pullrequest"):
sync_gitlab_issue(GitlabClient(url, token, upstream), iid, upstream)
log.info("Note was not added to an issue or merge request. Skipping note event.")

Comment on lines +80 to +81
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there have been a couple of conditional return statements before this point? (That is, it looks to me like we might indeed have added the note and still gotten to this log.info() call.)


def handle_gitlab_mr(body, headers, config, suffix):
"""
Handle Gitlab merge request from FedMsg.

:param Dict body: FedMsg Message body
:param Dict body: FedMsg Message headers
:param Dict config: Config File
:param String suffix: FedMsg suffix
"""
upstream = body["project"]["path_with_namespace"]
url = headers["x-gitlab-instance"]
token = config["sync2jira"].get("github_token")
labels = {label["title"] for label in body.get("labels", [])}
iid = body.get("object_attributes").get("iid")

if should_sync(upstream, labels, config, "pullrequest"):
sync_gitlab_mr(GitlabClient(url, token, upstream), iid, upstream, config)


def sync_gitlab_issue(client, iid, upstream, config):
gitlab_issue = client.fetch_issue(iid)
comments = gitlab_issue.notes.list(all=True)

issue = i.Issue.from_gitlab(gitlab_issue, comments, upstream, config)
d_issue.sync_with_jira(issue, config)


def sync_gitlab_mr(client, iid, upstream, config):
gitlab_mr = client.fetch_mr(iid)
comments = gitlab_mr.notes.list(all=True)

mr = i.PR.from_gitlab(gitlab_mr, comments, upstream, config)
d_pr.sync_with_jira(mr, config)
Comment on lines +114 to +115
Copy link
Collaborator

@webbnh webbnh Oct 10, 2025

Choose a reason for hiding this comment

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

You should probably name the local variable pr instead of mr, given that its value is returned by a method on the i.PR class and you're handing it off to a function in the d_pr module. 🙂



handlers = {
"gitlab.issues": handle_gitlab_issue,
"gitlab.issue_comment": handle_gitlab_mr,
"gitlab.note": handle_gitlab_note,
}


def get_handler_for(suffix, topic, idx):
"""
Function to check if a handler for given suffix is configured
:param String suffix: Incoming suffix
:param String topic: Topic of incoming message
:param String idx: Id of incoming message
:returns: Handler function if configured for suffix. Otherwise None.
"""
if suffix in handlers:
return handlers.get(suffix)
log.info("No gitlab handler for %r %r %r", suffix, topic, idx)
return None
Comment on lines +133 to +136
Copy link
Collaborator

@webbnh webbnh Oct 10, 2025

Choose a reason for hiding this comment

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

Admittedly, the lookup is cheap because the table is tiny, but, as a matter of principle, we shouldn't do it twice in a row:

Suggested change
if suffix in handlers:
return handlers.get(suffix)
log.info("No gitlab handler for %r %r %r", suffix, topic, idx)
return None
handler = handlers.get(suffix)
if not handler:
log.info("No gitlab handler for %r %r %r", suffix, topic, idx)
return handler

And, as I mentioned in the GitHub analog for this function, we probably shouldn't be logging this here. (The log message shouldn't need to mention "gitlab", because that will be in the suffix value, anyway.)

Loading