8000 Don't expect commit_id for PullReview by pabelanger · Pull Request #944 · sigmavirus24/github3.py · GitHub
[go: up one dir, main page]
Skip to content

Don't expect commit_id for PullReview #944

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

Jump to bottom
Merged
merged 1 commit into from
May 31, 2019
Merged

Don't expect commit_id for PullReview #944

merged 1 commit into from
May 31, 2019

Conversation

pabelanger
Copy link
Contributor

In some cases, a PullReview may not have a commit_id (I am not sure why
that is). In this case, we should not always expect it to be there.

2019-05-30 21:20:25,678 ERROR zuul.GithubEventConnector: Exception moving GitHub event:
Traceback (most recent call last):
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/github3/models.py", line 48, in init
self._update_attributes(json)
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/github3/pulls.py", line 961, in _update_attributes
self.commit_id = review["commit_id"]
KeyError: 'commit_id'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 484, in run
GithubEventProcessor(self, data).run()
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 218, in run
self._handle_event()
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 265, in _handle_event
refresh=True)
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 903, in _getChange
self._updateChange(change)
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 1008, in _updateChange
change.number)
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 1246, in getPullReviews
revs = [review.as_dict() for review in pr_obj.reviews()]
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 1246, in
revs = [review.as_dict() for review in pr_obj.reviews()]
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/github3/structs.py", line 122, in iter
yield cls(i)
File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/github3/models.py", line 50, in init
raise exceptions.IncompleteResponse(json, kerr)
github3.exceptions.IncompleteResponse: None The library was expecting more data in the response (KeyError('commit_id',)). Either GitHub modified it's response body, or your token is not prop
erly scoped to retrieve this information.

https://api.github.com/repos/ansible/ansible/pulls/45469/reviews

Signed-off-by: Paul Belanger pabelanger@redhat.com

omgjlk
omgjlk requested changes May 30, 2019
View reviewed changes
Copy link
Collaborator
@omgjlk omgjlk left a comment

Choose a reason for hiding this comment

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

This is a simple code change, and the code itself looks fine. I want to see the docstring updated to inform consumers why this value might be None.

I've checked with GitHub and it is expected that this can be None, and we're confirming the scenario in which it would be (current assumption is that it was a review on a commit that was force pushed out of the branch).

@sigmavirus24
Copy link
Owner

Can you ask GitHub support for an explanation?

@omgjlk
Copy link
Collaborator
omgjlk commented May 30, 2019

Can you ask GitHub support for an explanation?

I have an open request internally for more information.

@pabelanger
Copy link
Contributor Author

Pull-request updated, HEAD is now c096b61

@pabelanger pabelanger force-pushed the temp/missing-commit-id branch from e88fa45 to c096b61 Compare May 31, 2019 01:53
omgjlk
omgjlk reviewed May 31, 2019
View reviewed changes
omgjlk
omgjlk reviewed May 31, 2019
View reviewed changes
Copy link
Collaborator
@omgjlk omgjlk left a comment

Choose a reason for hiding this comment

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

This is good enough as is, but I'm going to wait to hear from GitHub on the real reasoning. It may. not be until next week.

omgjlk
omgjlk approved these changes May 31, 2019
View reviewed changes
Copy link
Collaborator
@omgjlk omgjlk left a comment

Choose a reason for hiding this comment

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

Oops, changing to approve to clear the previous rejection. The suggested change does not block the PR.

@pabelanger
Don't expect commit_id for PullReview
2ce81e7 
In some cases, a PullReview may not have a commit_id (I am not sure why
that is). In this case, we should not always expect it to be there.

  2019-05-30 21:20:25,678 ERROR zuul.GithubEventConnector: Exception moving GitHub event:
  Traceback (most recent call last):
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/github3/models.py", line 48, in __init__
      self._update_attributes(json)
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/github3/pulls.py", line 961, in _update_attributes
      self.commit_id = review["commit_id"]
  KeyError: 'commit_id'

  During handling of the above exception, another exception occurred:

  Traceback (most recent call last):
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 484, in run
      GithubEventProcessor(self, data).run()
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 218, in run
      self._handle_event()
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 265, in _handle_event
      refresh=True)
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 903, in _getChange
      self._updateChange(change)
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 1008, in _updateChange
      change.number)
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 1246, in getPullReviews
      revs = [review.as_dict() for review in pr_obj.reviews()]
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/zuul/driver/github/githubconnection.py", line 1246, in <listcomp>
      revs = [review.as_dict() for review in pr_obj.reviews()]
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/github3/structs.py", line 122, in __iter__
      yield cls(i)
    File "/opt/venv/zuul-3.8.1/lib/python3.6/site-packages/github3/models.py", line 50, in __init__
      raise exceptions.IncompleteResponse(json, kerr)
  github3.exceptions.IncompleteResponse: None The library was expecting more data in the response (KeyError('commit_id',)). Either GitHub modified it's response body, or your token is not prop
  erly scoped to retrieve this information.

  https://api.github.com/repos/ansible/ansible/pulls/45469/reviews

Signed-off-by: Paul Belanger <pabelanger@redhat.com>
@pabelanger
Copy link
Contributor Author

Pull-request updated, HEAD is now 2ce81e7

@pabelanger pabelanger force-pushed the temp/missing-commit-id branch from c096b61 to 2ce81e7 Compare May 31, 2019 11:09
@sigmavirus24 sigmavirus24 merged commit 473312a into sigmavirus24:master May 31, 2019
@omgjlk
Copy link
Collaborator
omgjlk commented May 31, 2019

Confirmed from internal, this is due to commit_id being lost due to force push.

We're unable to find the head_sha for some PRReviews due to force pushes making that commit no longer referenced, and eventually cleaned up in the repo network. This allows us to still return the review, even if the original commit no longer exists.

@pabelanger pabelanger mentioned this pull request Jun 24, 2019
Closed
@omgjlk omgjlk mentioned this pull request Jan 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omgjlk omgjlk omgjlk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
< 43A2 /option>
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pabelanger @sigmavirus24 @omgjlk
0