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 Check suite + check run events #26

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

AlistairB
Copy link
Contributor

@AlistairB AlistairB commented Feb 18, 2020

Very work in progress. I prefer to create the PR early as I work so I can add notes as I go. I'll clean up the git history when I am done. Ready for review.

Notes

Issue reference:
Closes #25

Submission Checklist:

  • Have you followed the guidelines in our Contributing document (for example, is your tree a clean merge)?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your submission build?
  • Does your submission pass tests?
  • Have you run lints on your code locally prior to submission? I see no lints related to my additions.
  • Have you updated all of the cabal/nix infrastructure? N/A
  • Is this a breaking change? Have you discussed this? Not breaking. Only adding new things
  • Add documentation for new types.

@kvanbere
Copy link
Member

Thanks, I'm watching this.

@AlistairB
Copy link
Contributor Author

AlistairB commented Feb 18, 2020

@donkeybonks I've been going down the path of creating BlahSimple data types for where the event payload does not match those already existing. However, it seems that many are subtly different that this feels a bit unwieldy.

Specifically CheckSuiteEvent requires different versions of:

  • HookCommit - The url is not included (in head_commit).
  • HookInstallation - Only the id is included.
  • HookPullRequest - A much smaller set.
  • PullRequestTarget - User and label not included.
  • HookRepository - A very simple version.

I can see some similarity between the representation of CheckSuiteEvent and CheckRunEvent. I think instead I will create HookChecksPullRequest and so on instead. Does that sound reasonable?

@kvanbere
Copy link
Member

Nevermind the last comment, your latest commit addresses it perfectly.

@AlistairB
Copy link
Contributor Author

@donkeybonks I'm pretty happy with this so far for CheckSuiteEvent. I'm going to proceed with adding CheckRunEvent (I prefer these in the same PR as check run may require tweaks to the shared checks data types).

Currently the check suite test for GHC 7.8 is failing with but got: Left "when expecting a Integral, encountered String instead". I'm struggling to pinpoint the issue. I don't suppose you have seen these failures on 7.8 before? I'm guessing it is due to a bug in an old version of aeson.

@kvanbere
Copy link
Member

kvanbere commented Feb 19, 2020

If the failures are the same or less than the test suite currently (there are currently a few accepted failures), then it is fine.

However, as I understand it, there are a few people using this package with GHC-7.8 in a commercial setting, hence my stubbornness to continue supporting it and not drop it in a hope that they eventually swap back to HEAD from their fork.

@AlistairB
Copy link
Contributor Author

AlistairB commented Feb 19, 2020

@donkeybonks No it does appear that it is specifically failing on the DecodeEvents can decode CheckSuiteEvent test only for GHC 7.8 and is not an accepted failure. No worries, I'll see if I can figure out what it is failing on.

@AlistairB
Copy link
Contributor Author

Hi @donkeybonks , I think this should be good now. Did you want to me to squash it into a single commit?

While I'm here, are you doing haskell in Melbourne? 😄 I'm Melbourne based. Always curious to hear about companies in Melbourne doing haskell.

@AlistairB AlistairB marked this pull request as ready for review February 19, 2020 06:21
@kvanbere
Copy link
Member

kvanbere commented Feb 19, 2020

Hi, yes please, squash it and we'll review it all together :)

I actually write firmware for electric trucks at a startup in South Dandenong (https://sea-electric.com), but I don't use Haskell at work. I noticed there's a clothing brand in Brighton that's hiring a senior Haskell technical lead, but unfortunately that's not me!

I didn't quite manage to take @onrock-eng quite off the ground contracting on Haskell alone - these days still taking mostly JS/CSS jobs, and still only on the side, although it's getting there.

@AlistairB
Copy link
Contributor Author

Done. Ah fair enough. Best of luck with it. I'm also trying my hand at a Haskell startup.

Copy link
Member

@kvanbere kvanbere left a comment

Choose a reason for hiding this comment

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

Perfect.

@kvanbere
Copy link
Member

Did you want to add yourself to AUTHORS, package.yaml and then rebuild the cabal file as well?

@kvanbere
Copy link
Member

Or I can merge this and you can do that as a separate PR if you’d like.

@AlistairB
Copy link
Contributor Author

Sure thanks! I pushed as a separate commit. Can squash it with the other commit if you need.

@kvanbere kvanbere merged commit ccc6847 into cuedo:master Feb 19, 2020
@kvanbere
Copy link
Member

Thank you! Merged with pleasure

@kvanbere
Copy link
Member

Once I get an opportunity tomorrow evening, I will update the changelog, bump the version and if all goes well and all the CI are still happy, release this to Hackage.

@AlistairB AlistairB changed the title Check suite + check run events Add Check suite + check run events Feb 19, 2020
@AlistairB AlistairB deleted the check-suite-events branch February 19, 2020 23:45
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.

Support the CheckSuite + CheckRun webhook events
2 participants