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 5 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"

4 changes: 2 additions & 2 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ LABEL com.github.actions.color="green"
RUN apk add --no-cache --upgrade expat libuuid

COPY python/requirements.txt /action/
RUN apk add --no-cache build-base libffi-dev; \
RUN apk add --no-cache build-base libffi-dev git; \
Copy link

Choose a reason for hiding this comment

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

31% of developers fix this issue

DL3013: Pin versions in pip. Instead of pip install <package> use pip install <package>==<version> or pip install --requirement <requirements file>


ℹ️ 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.

Copy link

Choose a reason for hiding this comment

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

12% of developers fix this issue

DL3018: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>


ℹ️ 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.

pip install --upgrade --force --no-cache-dir pip && \
pip install --upgrade --force --no-cache-dir -r /action/requirements.txt; \
apk del build-base libffi-dev
apk del build-base libffi-dev git

COPY python/publish /action/publish
COPY python/publish_test_results.py /action/
Expand Down
2 changes: 1 addition & 1 deletion composite/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ runs:
continue-on-error: true
with:
path: ${{ steps.os.outputs.pip-cache }}
key: enricomi-publish-action-${{ runner.os }}-${{ runner.arch }}-pip-${{ steps.python.outputs.version }}-b8e8abb8483d74cfe629dd253143f067
key: enricomi-publish-action-${{ runner.os }}-${{ runner.arch }}-pip-${{ steps.python.outputs.version }}-9b4d44cd08298a16faf5caff0672dc90

- name: Install Python dependencies
run: |
Expand Down
53 changes: 14 additions & 39 deletions python/publish/publisher.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@
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
from github.CheckRun import CheckRun
from github.CheckRunAnnotation import CheckRunAnnotation
from github.PullRequest import PullRequest
from github.PullRequestComment import PullRequestComment
Copy link

Choose a reason for hiding this comment

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

8% of developers fix this issue

E0401: Unable to import 'github.PullRequestComment'


ℹ️ 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.

Copy link

Choose a reason for hiding this comment

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

10% of developers fix this issue

reportMissingImports: Import "github.PullRequestComment" could not be resolved


ℹ️ 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.

from github.IssueComment import IssueComment
Copy link

Choose a reason for hiding this comment

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

9% of developers fix this issue

F401: 'github.IssueComment.IssueComment' imported but unused


ℹ️ 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.

Copy link

Choose a reason for hiding this comment

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

21% of developers fix this issue

vulture-90: unused import 'IssueComment'


ℹ️ 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.


from publish import __version__, comment_mode_off, digest_prefix, restrict_unicode_list, \
Expand Down Expand Up @@ -654,7 +655,7 @@ def do_stats_require_comment(earlier_stats_require: Optional[bool], stats_requir

return False

def get_latest_comment(self, pull: PullRequest) -> Optional[IssueComment]:
def get_latest_comment(self, pull: PullRequest) -> Optional[PullRequestComment]:
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 (84 > 79 characters)

❗❗ 2 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding
File Path Line Number
python/publish/publisher.py 706
python/publish/publisher.py 718

Visit the Lift Web Console to find more details in your report.


ℹ️ 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.

# get comments of this pull request
comments = self.get_pull_request_comments(pull, order_by_updated=True)

Expand All @@ -665,11 +666,10 @@ 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 comments[-1]

def reuse_comment(self, comment: IssueComment, body: str):
def reuse_comment(self, comment: PullRequestComment, body: str):
if ':recycle:' not in body:
body = f'{body}\n:recycle: This comment has been updated with latest results.'

Expand Down Expand Up @@ -703,41 +703,16 @@ 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 = ''
def get_pull_request_comments(self, pull: PullRequest, order_by_updated: bool) -> List[PullRequestComment]:
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')
return list(pull.get_comments(sort="updated_at", direction="asc"))
else:
return list(pull.get_comments())

def get_action_comments(self, comments: List[Mapping[str, Any]], is_minimized: Optional[bool] = False):
def get_action_comments(self, comments: List[PullRequestComment]):
comment_body_start = f'## {self._settings.comment_title}\n'
comment_body_indicators = ['\nresults for commit ', '\nResults for commit ']
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)])
2 changes: 1 addition & 1 deletion python/requirements-direct.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ humanize==3.14.0
junitparser==3.1.0
lxml==4.9.1
psutil==5.9.5
PyGithub==1.58.2
PyGithub @ git+https://github.com/pygithub/pygithub.git@daf62bd4610ba4c43ae8ced66ced3ff722c30cf0
requests==2.31.0
urllib3==1.26.16
2 changes: 1 addition & 1 deletion python/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ junitparser==3.1.0
future==0.18.3
lxml==4.9.1
psutil==5.9.5
PyGithub==1.58.2
PyGithub @ git+https://github.com/pygithub/pygithub.git@daf62bd4610ba4c43ae8ced66ced3ff722c30cf0
Deprecated==1.2.13
wrapt==1.15.0
PyJWT==2.4.0
Expand Down
143 changes: 50 additions & 93 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 @@ -2507,118 +2512,82 @@ def exception(base, head):

def do_test_get_pull_request_comments(self, order_updated: bool):
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_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
self.assertEqual(comments, response)
return pr

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'
' }'
' }'
' }'
' }'
'}'
}
)
pr = self.do_test_get_pull_request_comments(order_updated=False)
pr.get_comments.assert_called_once_with()

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'
' }'
' }'
' }'
' }'
'}'
}
)
pr = self.do_test_get_pull_request_comments(order_updated=True)
pr.get_comments.assert_called_once_with(sort='updated_at', direction='asc')

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 +2597,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 +2609,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)