Skip to content
This repository was archived by the owner on May 5, 2021. It is now read-only.

Include pull request reviews in database #128

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

MaineC
Copy link
Contributor

@MaineC MaineC commented Aug 3, 2018

Problem description: When commenting on pull requests, Github offers the option to bundle several comments together into a pull request review. As far as I can see, those reviews don't make it into the database so far. This PR adds the results of the https://octokit.github.io/octokit.rb/Octokit/Client/PullRequests.html#pull_requests_comments-instance_method call.

See MaineCSandboxOrg/sandbox#1 for an example some of the various types of comments that can be made on a pull request.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

xml_doc = LibXML::XML::Document.file(inputFile)
stylesheet = LibXSLT::XSLT::Stylesheet.new( LibXML::XML::Document.file(xsltFile))
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for this change? i.e. a nefarious bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Only me forgetting to revert things I changed while debugging a weird combination of Ubuntu/ xslt/ ruby:

xml4r/libxslt-ruby#13

tonyfg/libxslt-ruby#1

@@ -69,6 +69,10 @@ def github_sync(context, run_one)
# TODO: Move to being based on the async code
sync_issue_comments(context, sync_db)
end
if(not(run_one) or run_one=='github-sync/pr-reviews')
Copy link
Contributor

Choose a reason for hiding this comment

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

Mental note that I'll also need to implement the async side of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to help here - it will take me a while to figure things out though: My Ruby experience is pretty much non-existent ;)

@hyandell
Copy link
Contributor

Thank you Isabel - sorry for the delay :)

Just the one question added, and I need to test this out, including seeing what happens when it's run with --xsync.

@MaineC
Copy link
Contributor Author

MaineC commented Aug 31, 2018

Thank you Isabel - sorry for the delay :)

No need for sorry - I know what busy schedules look like.

Just the one question added, and I need to test this out, including seeing what happens when it's run
with --xsync.

Ah - so xsync is what triggers the -tng code path to be executed :)

@MaineC
Copy link
Contributor Author

MaineC commented Sep 19, 2018

I have simplified the implementation quite a bit (and discovered a bug in the earlier implementation with timestamp handling in my original patch). I also added support for synching review comments when working with --xsync.

If you want to quickly check things, feel free to use https://github.com/MaineCSandboxOrg/Sandbox which has three comments on a pull request:

  1. A regular comment.
  2. A comment that was added to a PR as part of a review.
  3. A comment that is attached to a PR review and was added before hitting submit.

Before the patch above, only 1. will be stored. After the patch, 1. and 3. will be stored.

This extends github-sync to also store pull request reviews in the database.
Review comments are being stored in the same table as regular issue comments.

This change comes with support for both, sync and async Github synchronisation.
Retrieval and storage of review specific comments has been folded into the
github-sync/issue-comments phase - with both, regular comments and review
comments in the same table, this makes timestamp handling easier.
@hyandell
Copy link
Contributor

Thanks for waiting Isabel - the latest code worked great for me. Using this amazon-archives/serverless-image-resizing#65 I went from 5 saved comments to 11, and saw no errors.

@hyandell hyandell merged commit cd84433 into amzn:master Sep 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants