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

Support webhook-2fm style messages #281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Support webhook-2fm style messages #281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions sync2jira/compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@


def extract_message_body(msg):
""" Strip off the message envelope.

Handle both fedmsg and fedora-messaging style message bodies.
"""

if 'body' in msg:
return msg['body']
elif 'msg' in msg:
return msg['msg']
else:
raise KeyError(f"Unrecognized message format with keys {msg.keys()}. Expected either 'msg' or 'body'")
Comment on lines +9 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.

Alternatively,

Suggested change
if 'body' in msg:
return msg['body']
elif 'msg' in msg:
return msg['msg']
else:
raise KeyError(f"Unrecognized message format with keys {msg.keys()}. Expected either 'msg' or 'body'")
body = msg.get("body", msg.get("msg"))
if body:
return body
raise KeyError(f"Unrecognized message format with keys {msg.keys()}. Expected either 'msg' or 'body'")

This avoids duplicate key look-ups, and it also raises an error if the value exists but is empty. (If you don't like this last part, you can change the if test to check for None explicitly to allow it to return an empty body.)

16 changes: 13 additions & 3 deletions sync2jira/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
from requests_kerberos import HTTPKerberosAuth, OPTIONAL

# Local Modules
import sync2jira.compat as c
import sync2jira.upstream_issue as u_issue
import sync2jira.upstream_pr as u_pr
import sync2jira.downstream_issue as d_issue
Expand All @@ -61,6 +62,10 @@
# Issue related handlers
issue_handlers = {
# GitHub
# New webhook-2fm topics
'github.issues': u_issue.handle_github_message,
'github.issue_comment': u_issue.handle_github_message,
# Old github2fedmsg topics
'github.issue.opened': u_issue.handle_github_message,
'github.issue.reopened': u_issue.handle_github_message,
'github.issue.labeled': u_issue.handle_github_message,
Expand All @@ -77,6 +82,10 @@
# PR related handlers
pr_handlers = {
# GitHub
# New webhook-2fm topics
'github.pull_request': u_pr.handle_github_message,
'github.issue_comment': u_pr.handle_github_message,
# Old github2fedmsg topics
'github.pull_request.opened': u_pr.handle_github_message,
'github.pull_request.edited': u_pr.handle_github_message,
'github.issue.comment': u_pr.handle_github_message,
Expand Down Expand Up @@ -276,8 +285,8 @@ def initialize_recent(config):

# Deal with the message
log.debug("Handling %r %r", suffix, entry['topic'])
msg = entry['msg']
handle_msg({'msg': msg}, suffix, config)
body = c.extract_message_body(entry)
handle_msg({'body': body}, suffix, config)


def handle_msg(msg, suffix, config):
Expand All @@ -292,7 +301,8 @@ def handle_msg(msg, suffix, config):
# GitHub '.issue.' is used for both PR and Issue
# Check for that edge case
if suffix == 'github.issue.comment':
if 'pull_request' in msg['msg']['issue'] and msg['msg']['action'] != 'deleted':
body = c.extract_message_body(msg)
Comment on lines 303 to +304
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than adding the call to extract_message_body() here, consider changing the function signature to receive the body directly instead of the message wrapper for it. This will save the dict instantiation at line 289 at the possible cost of having to add a call to extract_message_body() in listen() at line 172 (but, since you would be removing the call from here, that would be a net-zero cost and an overall savings, at the cost of an extra line of code change).

if 'pull_request' in body['issue'] and body['action'] != 'deleted':
# pr_filter turns on/off the filtering of PRs
pr = issue_handlers[suffix](msg, config, pr_filter=False)
if not pr:
Expand Down
8 changes: 5 additions & 3 deletions sync2jira/upstream_issue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import requests
from github import Github

import sync2jira.compat as c
import sync2jira.intermediary as i


Expand Down Expand Up @@ -121,8 +122,9 @@ def handle_github_message(msg, config, pr_filter=True):
:returns: Issue object
:rtype: sync2jira.intermediary.Issue
"""
owner = msg['msg']['repository']['owner']['login']
repo = msg['msg']['repository']['name']
body = c.extract_message_body(msg)
Comment on lines 124 to +125
Copy link
Collaborator

@webbnh webbnh Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ditto the previous comment.

owner = body['repository']['owner']['login']
repo = body['repository']['name']
upstream = '{owner}/{repo}'.format(owner=owner, repo=repo)

mapped_repos = config['sync2jira']['map']['github']
Expand All @@ -141,7 +143,7 @@ def handle_github_message(msg, config, pr_filter=True):
.get('github', {})\
.get(upstream, {})

issue = msg['msg']['issue']
issue = body['issue']
for key, expected in _filter.items():
if key == 'labels':
# special handling for label: we look for it in the list of msg labels
Expand Down
8 changes: 5 additions & 3 deletions sync2jira/upstream_pr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

from github import Github

import sync2jira.compat as c
import sync2jira.intermediary as i
import sync2jira.upstream_issue as u_issue

Expand All @@ -38,8 +39,9 @@ def handle_github_message(msg, config, suffix):
:returns: Issue object
:rtype: sync2jira.intermediary.PR
"""
owner = msg['msg']['repository']['owner']['login']
repo = msg['msg']['repository']['name']
body = c.extract_message_body(msg)
Comment on lines 41 to +42
Copy link
Collaborator

@webbnh webbnh Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ditto the previous comment.

owner = body['repository']['owner']['login']
repo = body['repository']['name']
upstream = '{owner}/{repo}'.format(owner=owner, repo=repo)

mapped_repos = config['sync2jira']['map']['github']
Expand All @@ -50,7 +52,7 @@ def handle_github_message(msg, config, suffix):
log.debug("%r not in Github PR map: %r", upstream, mapped_repos.keys())
return None

pr = msg['msg']['pull_request']
pr = body['pull_request']
github_client = Github(config['sync2jira']['github_token'])
reformat_github_pr(pr, upstream, github_client)
return i.PR.from_github(upstream, pr, suffix, config)
Expand Down
2 changes: 1 addition & 1 deletion tests/test_main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ def test_initialize_recent(self,
m.initialize_recent(self.mock_config)

# Assert everything was called correctly
mock_handle_msg.assert_called_with({'msg': 'mock_msg'}, 'github.issue.comment', self.mock_config)
mock_handle_msg.assert_called_with({'body': 'mock_msg'}, 'github.issue.comment', self.mock_config)

@mock.patch(PATH + 'handle_msg')
@mock.patch(PATH + 'query')
Expand Down
84 changes: 71 additions & 13 deletions tests/test_upstream_issue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def setUp(self):
self.mock_github_comment.created_at = 'mock_created_at'

# Mock GitHub Message
self.mock_github_message = {
self.old_style_mock_github_message = {
'msg': {
'repository': {
'owner': {
Expand All @@ -63,6 +63,29 @@ def setUp(self):
}
}
}
self.new_style_mock_github_message = {
'body': {
Comment on lines +66 to +67
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is new_style_mock_github_message supposed to be identical to old_style_mock_github_message except for the body vs. msg key? If so, I recommend defining the body separately and using that definition to define these two attributes (or to define one attribute using the other), to avoid repeating ourself.

'repository': {
'owner': {
'login': 'org'
},
'name': 'repo'
},
'issue': {
'filter1': 'filter1',
'labels': [{'name': 'custom_tag'}],
'comments': ['some_comments!'],
'number': 'mock_number',
'user': {
'login': 'mock_login'
},
'assignees': [{'login': 'mock_login'}],
'milestone': {
'title': 'mock_milestone'
}
}
}
}

# Mock GitHub issue
self.mock_github_issue = MagicMock()
Expand Down Expand Up @@ -460,11 +483,11 @@ def test_handle_github_message_not_in_mapped(self,
This function tests 'handle_github_message' where upstream is not in mapped repos
"""
# Set up return values
self.mock_github_message['msg']['repository']['owner']['login'] = 'bad_owner'
self.old_style_mock_github_message['msg']['repository']['owner']['login'] = 'bad_owner'

# Call the function
response = u.handle_github_message(
msg=self.mock_github_message,
msg=self.old_style_mock_github_message,
config=self.mock_config
)

Expand All @@ -482,11 +505,11 @@ def test_handle_github_message_pull_request(self,
This function tests 'handle_github_message' the issue is a pull request comment
"""
# Set up return values
self.mock_github_message['msg']['issue'] = {'pull_request': 'test'}
self.old_style_mock_github_message['msg']['issue'] = {'pull_request': 'test'}

# Call the function
response = u.handle_github_message(
msg=self.mock_github_message,
msg=self.old_style_mock_github_message,
config=self.mock_config
)

Expand All @@ -502,11 +525,11 @@ def test_handle_github_message_bad_filter(self,
This function tests 'handle_github_message' where comparing the actual vs. filter does not equate
"""
# Set up return values
self.mock_github_message['msg']['issue']['filter1'] = 'filter2'
self.old_style_mock_github_message['msg']['issue']['filter1'] = 'filter2'

# Call function
response = u.handle_github_message(
msg=self.mock_github_message,
msg=self.old_style_mock_github_message,
config=self.mock_config
)
# Assert that calls were made correctly
Expand All @@ -520,11 +543,11 @@ def test_handle_github_message_bad_label(self,
This function tests 'handle_github_message' where comparing the actual vs. filter does not equate
"""
# Set up return values
self.mock_github_message['msg']['issue']['labels'] = [{'name': 'bad_label'}]
self.old_style_mock_github_message['msg']['issue']['labels'] = [{'name': 'bad_label'}]

# Call function
response = u.handle_github_message(
msg=self.mock_github_message,
msg=self.old_style_mock_github_message,
config=self.mock_config
)
# Assert that calls were made correctly
Expand All @@ -540,11 +563,11 @@ def test_handle_github_message_no_comments(self, mock_issue_from_github, mock_gi
# Set up return values
mock_issue_from_github.return_value = "Successful Call!"
mock_github.return_value = self.mock_github_client
self.mock_github_message['msg']['issue']['comments'] = 0
self.old_style_mock_github_message['msg']['issue']['comments'] = 0

# Call function
response = u.handle_github_message(
msg=self.mock_github_message,
msg=self.old_style_mock_github_message,
config=self.mock_config
)
# Assert that calls were made correctly
Expand All @@ -564,7 +587,42 @@ def test_handle_github_message_no_comments(self, mock_issue_from_github, mock_gi

@mock.patch(PATH + 'Github')
@mock.patch('sync2jira.intermediary.Issue.from_github')
def test_handle_github_message_successful(self,
def test_handle_old_style_github_message_successful(self,
mock_issue_from_github,
mock_github):
"""
This function tests 'handle_github_message' where everything goes smoothly!
"""
# Set up return values
mock_issue_from_github.return_value = "Successful Call!"
mock_github.return_value = self.mock_github_client

# Call function
response = u.handle_github_message(
msg=self.old_style_mock_github_message,
config=self.mock_config
)

# Assert that calls were made correctly
mock_issue_from_github.assert_called_with('org/repo',
{'labels': ['custom_tag'], 'number': 'mock_number',
'comments': [{'body': 'mock_body', 'name': unittest.mock.ANY,
'author': 'mock_username', 'changed': None,
'date_created': 'mock_created_at', 'id': 'mock_id'}],
'assignees': [{'fullname': 'mock_name'}],
'filter1': 'filter1', 'user':
{'login': 'mock_login', 'fullname': 'mock_name'},
'milestone': 'mock_milestone'}, self.mock_config)
mock_github.assert_called_with('mock_token', retry=5)
self.assertEqual('Successful Call!', response)
self.mock_github_client.get_repo.assert_called_with('org/repo')
self.mock_github_repo.get_issue.assert_called_with(number='mock_number')
self.mock_github_issue.get_comments.assert_any_call()
self.mock_github_client.get_user.assert_called_with('mock_login')

Comment on lines 588 to +622
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than cookie-cuttering this code, I suggest reusing the code in test_handle_new_style_github_message_successful() with each of the old and new style of message, to avoid the code duplication.

@mock.patch(PATH + 'Github')
@mock.patch('sync2jira.intermediary.Issue.from_github')
def test_handle_new_style_github_message_successful(self,
mock_issue_from_github,
mock_github):
"""
Expand All @@ -576,7 +634,7 @@ def test_handle_github_message_successful(self,

# Call function
response = u.handle_github_message(
msg=self.mock_github_message,
msg=self.new_style_mock_github_message,
config=self.mock_config
)

Expand Down
Loading