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

Add support for PR reviews preview #352

Merged
merged 1 commit into from
Sep 8, 2017
Merged

Conversation

stephenc
Copy link
Contributor

@reviewbybees

/*
* The MIT License
*
* Copyright (c) 2010, Kohsuke Kawaguchi
Copy link
Contributor

Choose a reason for hiding this comment

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

really?

@ghost
Copy link

ghost commented Mar 30, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

Copy link
Collaborator

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

The code mostly looks good to me 🐝 , there are minor improvement proposals.
@reviewbybees done

@@ -224,6 +228,27 @@ protected void wrapUp(GHPullRequestFileDetail[] page) {
}

/**
* Retrieves all the reviews associated to this pull request.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice2have: since

Copy link
Collaborator

Choose a reason for hiding this comment

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

And below

* Updates the comment.
*/
@Preview
@Deprecated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it worth adding dependency on accmod and adding @Restricted annotations instead

return new PagedIterable<GHPullRequestReview>() {
public PagedIterator<GHPullRequestReview> _iterator(int pageSize) {
return new PagedIterator<GHPullRequestReview>(root.retrieve()
.withPreview("application/vnd.github.black-cat-preview+json")
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding an entry in Previews class?

@porcelli
Copy link

Hi, I'm interested in this PR, actually I've even enchanced this with support of new Events (Status and PullRequestReview). But to submit my PR, I'd need this merged first.

@jglick
Copy link
Contributor

jglick commented May 19, 2017

I think only @kohsuke can do that.

@henryju
Copy link
Contributor

henryju commented Jun 30, 2017

Fixes #305

@PauloMigAlmeida
Copy link
Contributor

PauloMigAlmeida commented Jul 21, 2017

I have accidentally created a pull request for the same purpose (#366) and I must confess that I'm very surprised that they are similar in so many ways. Regardless of that, since this PR has been created before and has already been reviewed by a CloudBees employee, I will close mine in favour or this one.

I will do my review on this one too since we both spent a significant time working on this matter and I would love to see this merged in the short-term as some of the automation process I created at work would really benefit of it.

@kohsuke when you have a spare time, please merge it ;)

Copy link
Contributor

@PauloMigAlmeida PauloMigAlmeida left a comment

Choose a reason for hiding this comment

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

I don't think any of the points I mentioned is a blocker to get this merged as I have pointed out only minor changes.

@@ -300,4 +353,28 @@ private void fetchIssue() throws IOException {
fetchedIssueDetails = true;
}
}

private static class DraftReviewComment {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's just my 2 cents opinion but why don't you make this public and add the GH prefix on it too to make it standard with the majority of the other classes in the project.

}
return new Requester(root).method("POST")
.with("body", body)
//.with("event", event.name())
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong but if you aren't sending the event, then all of the reviews will be set to Pending state, right? If that's the case, why you have the event parameter in the first place?

I think we should let send it as it was specified in the method signature (2 cents opinion again)

.with("body", body)
//.with("event", event.name())
._with("comments", draftComments)
.withPreview("application/vnd.github.black-cat-preview+json")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no long a preview method, so you can remove this line

return createReview(body, event, Arrays.asList(comments));
}

@Preview
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no long a preview method, so you can remove this line

}

@Preview
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no long a preview method, so you can remove this line

* Dismisses this review.
*/
@Preview
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no long a preview method, so you can remove this line

public void dismiss(String message) throws IOException {
new Requester(owner.root).method("PUT")
.with("message", message)
.withPreview("application/vnd.github.black-cat-preview+json")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no long a preview method, so you can remove this line

public PagedIterator<GHPullRequestReviewComment> _iterator(int pageSize) {
return new PagedIterator<GHPullRequestReviewComment>(
owner.root.retrieve()
.withPreview("application/vnd.github.black-cat-preview+json")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no long a preview method, so you can remove this line

/**
* Obtains all the review comments associated with this pull request review.
*/
@Preview
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no long a preview method, so you can remove this line

* Obtains all the review comments associated with this pull request review.
*/
@Preview
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no long a preview method, so you can remove this line

@kohsuke kohsuke merged commit 4d277cc into hub4j:master Sep 8, 2017
@mattnelson
Copy link
Contributor

Hi, I'm interested in this PR, actually I've even enchanced this with support of new Events (Status and PullRequestReview). But to submit my PR, I'd need this merged first.

@porcelli Now that this is merged and released would you be able to issue your PR to add event support for PR reviews?

@PauloMigAlmeida
Copy link
Contributor

@mattnelson the new events (Status and PullRequestReview) that @porcelli mentioned were already merged in this PR: https://github.com/kohsuke/github-api/pull/363/files

@mattnelson
Copy link
Contributor

@PauloMigAlmeida thanks, I missed that.

I was specifically looking for the data binding in GHEventPayload as I was wanting to implement a hook on the PullRequestReview event. If this hasn't been implemented yet, I will look into it. The rest of the parts are already there, so it should be pretty strait forward.

@PauloMigAlmeida
Copy link
Contributor

PauloMigAlmeida commented Oct 3, 2017

Hi @mattnelson

If what you want it to implement a hook based on the PullRequestReview event from GHEvent class. This is what you can do:
PS.: This is a piece of code extracted from an internal system of ours that does exactly you just described.

public class GitConfigRepositoryOperation extends AbstractGitHubOperation<Void> {

    private GHRepository repository;
    private final static String webhookUrl;

    static{
        webhookUrl = KMSEnvironmentalVariableDecryptService.getInstance().decryptKey("GitWebhookUrl");
    }

    public GitConfigRepositoryOperation(GHRepository repository) throws IOException {
        super(repository.getFullName());
        this.repository = repository;
    }

    @Override
    public Void performOperation() throws Exception {

        // Configure Webhook
        Map<String, String> configMap = new HashMap<>(2);
        configMap.put("url", webhookUrl);
        configMap.put("content_type", "json");
        this.repository.createHook("web",
                configMap,
                Arrays.asList(
                    GHEvent.PULL_REQUEST,
                    GHEvent.PULL_REQUEST_REVIEW,
                    GHEvent.PULL_REQUEST_REVIEW_COMMENT,
                    GHEvent.PUSH,
                    GHEvent.CREATE
                ), true);

        return null;
    }

Hope that it helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants