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

Use REST API to get comments #467

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
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
2 changes: 1 addition & 1 deletion .github/upgrade-pip-packages.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ pip install --upgrade --force pip==22.0.0
pip install --upgrade --upgrade-strategy eager -r "$base/../python/requirements-direct.txt" -c "$base/../python/constraints.txt"

pip install pipdeptree
pipdeptree --packages="$(sed -e "s/;.*//" -e "s/=.*//g" "$base/../python/requirements-direct.txt" | paste -s -d ,)" --freeze > "$base/../python/requirements.txt"
pipdeptree --packages="$(sed -e "s/\s*;.*//" -e "s/\s*@.*//" -e "s/=.*//g" "$base/../python/requirements-direct.txt" | paste -s -d ,)" --freeze > "$base/../python/requirements.txt"

git diff "$base/../python/requirements.txt"

1 change: 1 addition & 0 deletions .github/workflows/ci-cd.yml
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ jobs:
json_suite_details: true
json_test_case_results: true
report_suite_logs: "any"
log_level: DEBUG

- name: JSON output
uses: ./misc/action/json-output
Expand Down
56 changes: 17 additions & 39 deletions python/publish/publisher.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import os
import re
from dataclasses import dataclass
from typing import List, Set, Any, Optional, Tuple, Mapping, Dict, Union, Callable
from typing import List, Any, Optional, Tuple, Mapping, Dict, Union, Callable
from copy import deepcopy

from github import Github, GithubException, UnknownObjectException
Expand Down Expand Up @@ -656,7 +656,7 @@ def do_stats_require_comment(earlier_stats_require: Optional[bool], stats_requir

def get_latest_comment(self, pull: PullRequest) -> Optional[IssueComment]:
# get comments of this pull request
comments = self.get_pull_request_comments(pull, order_by_updated=True)
comments = self.get_pull_request_comments(pull)

# get all comments that come from this action and are not hidden
comments = self.get_action_comments(comments)
Expand All @@ -665,9 +665,8 @@ def get_latest_comment(self, pull: PullRequest) -> Optional[IssueComment]:
if len(comments) == 0:
return None

# fetch latest action comment
comment_id = comments[-1].get("databaseId")
return pull.get_issue_comment(comment_id)
# return latest action comment
return sorted(comments, key=lambda comment: comment.updated_at, reverse=True)[-1]
Copy link

Choose a reason for hiding this comment

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

6% of developers fix this issue

E501: line too long (89 > 79 characters)


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


def reuse_comment(self, comment: IssueComment, body: str):
if ':recycle:' not in body:
Expand Down Expand Up @@ -703,41 +702,20 @@ def get_base_commit_sha(self, pull_request: PullRequest) -> Optional[str]:

return None

def get_pull_request_comments(self, pull: PullRequest, order_by_updated: bool) -> List[Mapping[str, Any]]:
order = ''
if order_by_updated:
order = ', orderBy: { direction: ASC, field: UPDATED_AT }'

query = dict(
query=r'query ListComments {'
r' repository(owner:"' + self._repo.owner.login + r'", name:"' + self._repo.name + r'") {'
r' pullRequest(number: ' + str(pull.number) + r') {'
f' comments(last: 100{order}) {{'
r' nodes {'
r' id, databaseId, author { login }, body, isMinimized'
r' }'
r' }'
r' }'
r' }'
r'}'
)

headers, data = self._req.requestJsonAndCheck(
"POST", self._settings.graphql_url, input=query
)

return data \
.get('data', {}) \
.get('repository', {}) \
.get('pullRequest', {}) \
.get('comments', {}) \
.get('nodes')
def get_pull_request_comments(self, pull: PullRequest) -> List[IssueComment]:
Copy link

Choose a reason for hiding this comment

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

6% of developers fix this issue

E501: line too long (81 > 79 characters)


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

comments = list(pull.get_issue_comments())
logger.debug(f'Found {len(comments)} comments for pull request {pull.number}')
Copy link

Choose a reason for hiding this comment

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

6% of developers fix this issue

E501: line too long (86 > 79 characters)


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

for comment in comments:
logger.debug(f'comment: {comment}')
return comments

def get_action_comments(self, comments: List[Mapping[str, Any]], is_minimized: Optional[bool] = False):
def get_action_comments(self, comments: List[IssueComment]):
comment_body_start = f'## {self._settings.comment_title}\n'
comment_body_indicators = ['\nresults for commit ', '\nResults for commit ']
for comment in comments:
logger.debug(f'login={comment.user.login}')
logger.debug(f'body={comment.body}')
return list([comment for comment in comments
if comment.get('author', {}).get('login') == self._settings.actor
and (is_minimized is None or comment.get('isMinimized') == is_minimized)
and comment.get('body', '').startswith(comment_body_start)
and any(indicator in comment.get('body', '') for indicator in comment_body_indicators)])
if comment.user.login == self._settings.actor
and comment.body.startswith(comment_body_start)
and any(indicator in comment.body for indicator in comment_body_indicators)])
150 changes: 48 additions & 102 deletions python/test/test_publisher.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@
import unittest
from collections.abc import Collection
from datetime import datetime, timezone
from typing import Optional, List, Mapping, Union, Any, Callable
from typing import Optional, List, Mapping, Union, Any, Callable, Dict

import github.CheckRun
import mock
from github import Github, GithubException
from github.PullRequestComment import PullRequestComment

from publish import __version__, comment_mode_off, comment_mode_always, \
comment_mode_changes, comment_mode_changes_failures, comment_mode_changes_errors, \
Expand Down Expand Up @@ -50,6 +51,10 @@ class CommentConditionTest:
current_has_errors: bool


def comment(data: Dict[str, Any]) -> PullRequestComment:
return PullRequestComment(None, {}, data, False)


class TestPublisher(unittest.TestCase):

@staticmethod
Expand Down Expand Up @@ -2505,120 +2510,73 @@ def exception(base, head):

self.assertEqual(None, result)

def do_test_get_pull_request_comments(self, order_updated: bool):
def test_get_pull_request_comments(self):
settings = self.create_settings()
comments = [mock.Mock()]

gh, gha, req, repo, commit = self.create_mocks(repo_name=settings.repo, repo_login='login')
req.requestJsonAndCheck = mock.Mock(
return_value=({}, {'data': {'repository': {'pullRequest': {'comments': {'nodes': ['node']}}}}})
)
pr = self.create_github_pr(settings.repo, number=1234)
pr.get_issue_comments = mock.Mock(return_value=comments)
publisher = Publisher(settings, gh, gha)

response = publisher.get_pull_request_comments(pr, order_by_updated=order_updated)
self.assertEqual(['node'], response)
return req

def test_get_pull_request_comments(self):
req = self.do_test_get_pull_request_comments(order_updated=False)
req.requestJsonAndCheck.assert_called_once_with(
'POST', 'https://the-github-graphql-url',
input={
'query': 'query ListComments {'
' repository(owner:"login", name:"owner/repo") {'
' pullRequest(number: 1234) {'
' comments(last: 100) {'
' nodes {'
' id, databaseId, author { login }, body, isMinimized'
' }'
' }'
' }'
' }'
'}'
}
)

def test_get_pull_request_comments_order_updated(self):
req = self.do_test_get_pull_request_comments(order_updated=True)
req.requestJsonAndCheck.assert_called_once_with(
'POST', 'https://the-github-graphql-url',
input={
'query': 'query ListComments {'
' repository(owner:"login", name:"owner/repo") {'
' pullRequest(number: 1234) {'
' comments(last: 100, orderBy: { direction: ASC, field: UPDATED_AT }) {'
' nodes {'
' id, databaseId, author { login }, body, isMinimized'
' }'
' }'
' }'
' }'
'}'
}
)
response = publisher.get_pull_request_comments(pr)
self.assertEqual(comments, response)
pr.get_issue_comments.assert_called_once_with()

comments = [
{
'id': 'comment one',
'author': {'login': 'github-actions'},
comment({
'id': 1,
'user': {'login': 'github-actions'},
'body': '## Comment Title\n'
'Results for commit dee59820.\u2003± Comparison against base commit 70b5dd18.\n',
'isMinimized': False
},
{
'id': 'comment two',
'author': {'login': 'someone else'},
}),
comment({
'id': 2,
'user': {'login': 'someone else'},
'body': '## Comment Title\n'
'more body\n'
'Results for commit dee59820.\u2003± Comparison against base commit 70b5dd18.\n',
'isMinimized': False
},
{
'id': 'comment three',
'author': {'login': 'github-actions'},
}),
comment({
'id': 3,
'user': {'login': 'github-actions'},
'body': '## Wrong Comment Title\n'
'more body\n'
'Results for commit dee59820.\u2003± Comparison against base commit 70b5dd18.\n',
'isMinimized': False
},
{
'id': 'comment four',
'author': {'login': 'github-actions'},
}),
comment({
'id': 4,
'user': {'login': 'github-actions'},
'body': '## Comment Title\n'
'more body\n'
'no Results for commit dee59820.\u2003± Comparison against base commit 70b5dd18.\n',
'isMinimized': False
},
{
'id': 'comment five',
'author': {'login': 'github-actions'},
}),
comment({
'id': 5,
'user': {'login': 'github-actions'},
'body': '## Comment Title\n'
'more body\n'
'Results for commit dee59820.\u2003± Comparison against base commit 70b5dd18.\n',
'isMinimized': True
},
{
'id': 'comment six',
'author': {'login': 'github-actions'},
}),
comment({
'id': 6,
'user': {'login': 'github-actions'},
'body': 'comment',
'isMinimized': True
},
}),
# earlier version of comments with lower case result and comparison
{
'id': 'comment seven',
'author': {'login': 'github-actions'},
comment({
'id': 7,
'user': {'login': 'github-actions'},
'body': '## Comment Title\n'
'results for commit dee59820\u2003± comparison against base commit 70b5dd18\n',
'isMinimized': False
},
}),
# comment of different actor
{
'id': 'comment eight',
'author': {'login': 'other-actor'},
comment({
'id': 8,
'user': {'login': 'other-actor'},
'body': '## Comment Title\n'
'Results for commit dee59820.\u2003± Comparison against base commit 70b5dd18.\n',
'isMinimized': False
},
}),
]

def test_get_action_comments(self):
Expand All @@ -2628,8 +2586,8 @@ def test_get_action_comments(self):

expected = [comment
for comment in self.comments
if comment.get('id') in ['comment one', 'comment five', 'comment seven']]
actual = publisher.get_action_comments(self.comments, is_minimized=None)
if comment.id in {1, 5, 7}]
actual = publisher.get_action_comments(self.comments)
self.assertEqual(3, len(expected))
self.assertEqual(expected, actual)

Expand All @@ -2640,19 +2598,7 @@ def test_get_action_comments_other_actor(self):

expected = [comment
for comment in self.comments
if comment.get('id') == 'comment eight']
actual = publisher.get_action_comments(self.comments, is_minimized=None)
if comment.id == 8]
actual = publisher.get_action_comments(self.comments)
self.assertEqual(1, len(expected))
self.assertEqual(expected, actual)

def test_get_action_comments_not_minimized(self):
settings = self.create_settings(actor='github-actions')
gh, gha, req, repo, commit = self.create_mocks()
publisher = Publisher(settings, gh, gha)

expected = [comment
for comment in self.comments
if comment.get('id') in ['comment one', 'comment seven']]
actual = publisher.get_action_comments(self.comments, is_minimized=False)
self.assertEqual(2, len(expected))
self.assertEqual(expected, actual)