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

SKARA-758: Add /check pull request command #954

Closed
wants to merge 3 commits into from
Closed

SKARA-758: Add /check pull request command #954

wants to merge 3 commits into from

Conversation

amCap1712
Copy link
Contributor

@amCap1712 amCap1712 commented Nov 17, 2020

Hi!
Kindly review the patch to allow manual execution of jcheck. I think it might be worthwhile to restrict this command to some specific people but I am not sure who that should be (maybe restricting to the author is enough but a reviewer might want to execute it as well in case its urgent).

I wanted to test this locally by setting up the skara bots on my own repository but was unable to figure out how to do so due to lack of documentation.
Thanks.
Regards,
Kartik


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Testing

Linux x64 Windows x64
Build / test ✔️ (1/1 passed) ✔️ (1/1 passed)

Issue

Download

$ git fetch https://git.openjdk.java.net/skara pull/954/head:pull/954
$ git checkout pull/954

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 17, 2020

👋 Welcome back amCap1712! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@amCap1712 amCap1712 closed this Nov 17, 2020
@amCap1712 amCap1712 reopened this Nov 17, 2020
@amCap1712 amCap1712 changed the title Add /check command to manually execute jcheck SKARA-758: Add /check pull request command Nov 17, 2020
@openjdk openjdk bot added the rfr label Nov 17, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 17, 2020

Webrevs

@edvbld
Copy link
Member

@edvbld edvbld commented Nov 17, 2020

Hi @amCap1712, thanks for working on this!

Please consider adding a unit test for the /check command, see e.g. SummaryTests.java for an example of a unit test for a pull request command.

Thanks,
Erik

@amCap1712
Copy link
Contributor Author

@amCap1712 amCap1712 commented Nov 20, 2020

Hi @edvbld,
Thanks for the review. I have added a unit test as you asked. Let me know if any other changes are required.
Regards,
Kartik.

@Override
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
try {
var workItem = new CheckWorkItem(bot, pr, e -> {});
Copy link
Member

@rwestberg rwestberg Nov 23, 2020

Choose a reason for hiding this comment

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

Don't think you actually have to do the check here, CommandWorkItem will schedule an additional check after every command anyway. But to force the check to run you'll need to ensure that the "metadata" of the existing check becomes invalid. If you update metadataComments in CheckWorkItem to add a special reply marker from the /check command, and ensure that the check command outputs it, I think thinks should work.

var editHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(editHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");
pr.addComment("/check");
Copy link
Member

@rwestberg rwestberg Nov 23, 2020

Choose a reason for hiding this comment

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

Looks like a good start, but it's possible that this test is a bit too simple. Would it still pass if you removed this line (as checking will occur anyway)?

@rwestberg
Copy link
Member

@rwestberg rwestberg commented Nov 23, 2020

I wanted to test this locally by setting up the skara bots on my own repository but was unable to figure out how to do so due to lack of documentation.

Yeah, sorry about that, it has not really been a priority as no-one else actually run them in "production". It can be a bit complicated to configure them properly for running stand-alone, but if you want to run a test against GitHub instead of the mock implementation that's a bit easier. If you want to try that I'll be happy to assist with the required configuration.

@amCap1712
Copy link
Contributor Author

@amCap1712 amCap1712 commented Nov 24, 2020

@rwestberg Yes, please. I plan to keep contributing in the future and having such a setup will help me to explore and understand the code much more quickly and in a better way. Hence, I think setting it up will be worth it. Thanks again for the help.

@rwestberg
Copy link
Member

@rwestberg rwestberg commented Nov 24, 2020

Sure, let's start with the required setup for running tests against GitHub (or GitLab) then! You can pass -Pcredentials=/path/to/credentials.json where the file contains something like this:

{
    "type": "github",
    "host": "https://github.com",
    "project": "rwestberg/skara-integration",
    "namespace": "github.com",
    "apps": [
        {
            "key": "certificates/sbot.private-key.pkcs8.pem",
            "id": "12837",
            "installation": "195957"
        },
        {
            "key": "certificates/sbot2.private-key.pkcs8.pem",
            "id": "18848",
            "installation": "379295"
        },
        {
            "key": "certificates/sbot3.private-key.pkcs8.pem",
            "id": "19394",
            "installation": "397926"
        }
    ]
}

Most tests require two or three different accounts in order to simulate various interactions, so that's why there are a list of them. Each account credential corresponds to a GitHub app that you can create on your account. The id can be seen after creating the app, the installation value can be found after the app is installed on your account. Finally, the key is the private key you associate with the GitHub app. The path is relative to the .json file. Note that it has to be in PEM format, you'll have to convert the file you get from GitHub.

@amCap1712
Copy link
Contributor Author

@amCap1712 amCap1712 commented Nov 25, 2020

I have created the 3 Github Apps and generated the .json file as you instructed. How should I proceed ?

@rwestberg
Copy link
Member

@rwestberg rwestberg commented Nov 25, 2020

Now you can use the credentials file when running the tests through Gradle, probably something like this (I usually run from IntelliJ):

sh gradlew test --tests *testCheckRun* -Pcredentials=/path/to/credentials.json

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 16, 2020

@amCap1712 This pull request has been inactive for more than 3 weeks and will be automatically closed if another 3 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 6, 2021

@amCap1712 This pull request has been inactive for more than 6 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

@bridgekeeper bridgekeeper bot closed this Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants