Add gitlab support #378

engelmi · 2025-10-09T13:59:54Z

Added initial gitlab integration (initialize part for gitlab and unit tests are currently missing).

Signed-off-by: Michael Engel <[email protected]>

webbnh

Hi @engelmi, this looks like a good start. You did note that it was a Draft, but I figured that, since you went to the trouble of posting it, I would review it for you. There's a good bit of change here, so I have a good number of comments. 🙂

webbnh · 2025-10-09T21:06:35Z

sync2jira/api/gitlab_client.py

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


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().

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

webbnh · 2025-10-09T21:25:06Z

sync2jira/api/gitlab_client.py

+    def fetch_issue(self, iid):
+        return self._project.issues.get(iid)


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....)

webbnh · 2025-10-10T20:05:26Z

sync2jira/main.py

+        except GithubException as e:
+            log.error("Unexpected GitHub error: %s", e)
+        except JIRAError as e:
+            log.error("Unexpected Jira error: %s", e)
+        except Exception as e:
+            log.exception("Unexpected error.", exc_info=e)


Should there be an except GitlabException clause here? Alternatively, since the log messages don't seem very different in the three existing cases, perhaps we should remove the special cases for GitHub and Jira and just let the catch-all handle those?

webbnh · 2025-10-10T20:11:38Z

sync2jira/handler/github.py

+    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


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

webbnh · 2025-10-10T20:16:47Z

sync2jira/handler/github.py

+    log.info("No github handler for %r %r %r", suffix, topic, idx)
+    return None


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.)

webbnh · 2025-10-10T22:37:13Z

tests/test_main.py

    @mock.patch.dict(
-        PATH + "issue_handlers", {"github.issue.comment": lambda msg, c: None}
+        HANDLER_PATH + "issue_handlers", {"github.issue.comment": lambda msg, c: None}
    )
-    @mock.patch(PATH + "u_issue")
-    @mock.patch(PATH + "d_issue")
+    @mock.patch(HANDLER_PATH + "u_issue")
+    @mock.patch(HANDLER_PATH + "d_issue")
    def test_handle_msg_no_issue(self, mock_d, mock_u):
        """
        Tests 'handle_msg' function where there is no issue
        """
+        mock_u.handle_github_message.return_value = None


If you're going to mock handle_issue_msg(), do we still need the patch at line 348?

webbnh · 2025-10-10T22:43:30Z

tests/test_main.py

-        mock_u.handle_github_message.assert_not_called()
+        mock_u.handle_github_message.assert_called_once()


This seems like a behavior change...and that raises a red flag....

Assuming that the old test was correct for the old implementation, why are we expecting the new implementation to call handle_github_message() in this scenario when the old implementation did not?...did the implementation change in some significant way, or has the test scenario been changed?

As a general rule, we should not be changing test scenarios. (We can add new ones, but, unless our interface contract has changed, we should not be fundamentally altering old ones.)

webbnh · 2025-10-10T22:45:56Z

tests/test_main.py

+        # Set up return values
+        mock_u.handle_github_message.return_value = "dummy_issue"


Given this mock, do we need the one at line 371?

webbnh · 2025-10-10T22:47:17Z

tests/test_main.py

+        """
+        Tests 'handle_issue_msg' function
+        """


This is a pretty minimal description...can you enlarge it a bit? (E.g., are we testing a success scenario or a failure scenario? Can you qualify what we're trying to demonstrate?)

(It looks like we're exercising the normal, success path....)

webbnh · 2025-10-10T22:51:06Z

tests/test_main.py

        """
-        Tests 'handle_msg' function
+        Tests 'handle_issue_msg' function
        """


This scenario seems to test the handle_pr_msg() function, not the issue one. (And, it looks like it tests the normal, success case, which would be worth mentioning.)

webbnh · 2025-10-14T20:04:23Z

/ok-to-test

webbnh · 2025-10-23T13:48:47Z

@engelmi, not to add to your load or anything...but I just stumbled across this page in the Sync2Jira documentation...it would be great if, as a part of this PR, you could check it over and make sure that it is still accurate or fix it up (source).

Thanks!

engelmi added 2 commits October 7, 2025 15:37

Refactored msg handler and moved github handler to dedicated directory

85e61fd

Signed-off-by: Michael Engel <[email protected]>

Added initial gitlab integration

2ce53e2

Signed-off-by: Michael Engel <[email protected]>

webbnh reviewed Oct 10, 2025

View reviewed changes

		def fetch_issue(self, iid):
		return self._project.issues.get(iid)

		log.info("No github handler for %r %r %r", suffix, topic, idx)
		return None

		mock_u.handle_github_message.assert_not_called()
		mock_u.handle_github_message.assert_called_once()

		# Set up return values
		mock_u.handle_github_message.return_value = "dummy_issue"

Add gitlab support #378

Are you sure you want to change the base?

Add gitlab support #378

Uh oh!

Conversation

engelmi commented Oct 9, 2025

Uh oh!

webbnh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webbnh Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webbnh commented Oct 14, 2025

Uh oh!

webbnh commented Oct 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

webbnh Oct 10, 2025 •

edited

Loading