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 ability to update a check run #980

Merged
merged 4 commits into from
Nov 24, 2020
Merged

Conversation

mrginglymus
Copy link
Contributor

Add functionality to update an existing check run

I have added a test which I fully expect to fail, as I have no idea where to even begin with implementing the wiremock backend - are they all manually crafted files?

@timja
Copy link
Collaborator

timja commented Nov 19, 2020

are they all manually crafted files?

they are recorded files mostly, possibly with small tweaks on top, I've invited you to the test org

@mrginglymus
Copy link
Contributor Author

Hmm, the tests for checks appear to use a repo owned by @jglick (along with a corresponding app).

@timja
Copy link
Collaborator

timja commented Nov 19, 2020

Hmm, the tests for checks appear to use a repo owned by @jglick (along with a corresponding app).

might be better to migrate them to the test org

@mrginglymus
Copy link
Contributor Author

Ok, I'll see what I can do 👍

@mrginglymus
Copy link
Contributor Author

Well that was churny :) All passing now @timja, thanks for sorting me out with the access.

@timja timja requested a review from bitwiseman November 19, 2020 15:31
Copy link
Collaborator

@timja timja left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@jglick jglick left a comment

Choose a reason for hiding this comment

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

WireMock in replay mode is basically unreviewable from my PoV; I find it more maintainable to write mocks programmatically. (example)

GHCheckRun checkRun = gitHub.getRepository("jglick/github-api-test")
.createCheckRun("foo", "4a929d464a2fae7ee899ce603250f7dab304bc4b")
GHCheckRun checkRun = getInstallationGithub().getRepository("hub4j-test-org/test-checks")
.createCheckRun("foo", "89a9ae301e35e667756034fdc933b1fc94f63fc1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you needed to update existing tests? Once the mocks are created they should continue to work so long as you are not actually changing the behavior of the existing APIs, which AFAICT you are not.

Copy link
Collaborator

@timja timja Nov 19, 2020

Choose a reason for hiding this comment

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

It was to make it easier to update them for others by moving the tests to the test org and an app that others can use I believe (could be a separate pr)

@mrginglymus
Copy link
Contributor Author

Hmm, from looking at how this might be used downstream, it could probably do with a GHRepository#updateCheckRun(long checkId) - does that seem reasonable? Otherwise it's only accessible from a GHCheckRun object.

@timja
Copy link
Collaborator

timja commented Nov 20, 2020

GHRepository

given that create is there https://github.com/hub4j/github-api/blob/master/src/main/java/org/kohsuke/github/GHRepository.java#L1957 I don't see why not

@mrginglymus mrginglymus requested a review from timja November 20, 2020 09:16
@timja
Copy link
Collaborator

timja commented Nov 20, 2020

@bitwiseman did you want to take a look at this?

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

I'll look at this tody

@bitwiseman bitwiseman merged commit 208904b into hub4j:master Nov 24, 2020
@mrginglymus mrginglymus deleted the check-patcher branch November 24, 2020 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants