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

OwnerType: add OwnerBot #399

Merged
merged 1 commit into from
Sep 29, 2019
Merged

Conversation

domenkozar
Copy link
Contributor

@TomMD
Copy link
Contributor

TomMD commented Sep 6, 2019

I'm very confused. The link is for github api v4 but this library is supporting v3. Is there a shift to support v4? Did I miss the concept of 'bot' defined in the v3 API? I suppose it is just unspecified but should appear somewhere like https://developer.github.com/v3/users/

@phadej
Copy link
Contributor

phadej commented Sep 6, 2019

v4 is GraphQL api, which we cannot really support with current design of library.

@domenkozar
Copy link
Contributor Author

This also appears on v3, but unfortunately there's not a single word mentioning it.

See type in https://api.github.com/users/bors[bot]

@domenkozar
Copy link
Contributor Author

@phadej @TomMD does that clear up confusion?

@phadej
Copy link
Contributor

phadej commented Sep 12, 2019

It does clear some confusion, but makes me wonder whether v3 stuff works by accident. Could you contact GitHub support and clarify (perfectly resulting into updated v3 documentation).

I said it before, and I repeat, tracking GitHub undocumented features is not worth anything. There should an URL to refer. And this library works with v3 API.

@domenkozar
Copy link
Contributor Author

@phadej it's part of github apps (which we still need to PR).

But even without other parts of the github apps API, if someone has github app configured, they can use bots to trigger webhooks that will error out in the current github.

See https://developer.github.com/apps/differences-between-apps/#machine-vs-bot-accounts

@domenkozar
Copy link
Contributor Author

@robbiemcmichael
Copy link
Contributor

robbiemcmichael commented Sep 12, 2019

GitHub Apps are still an API preview. There's been some previous discussion where there's been apprehension about incorporating API previews into this library in #351 and #367.

I started work on a separate library for API previews but haven't had any time to work on it recently. You're welcome to contribute or use that code for inspiration. I might try to clean up some of it this weekend. You'll probably also find #365 and #370 relevant for authenticating as a GitHub App.

@robbiemcmichael
Copy link
Contributor

Looking at this a bit more closely, it does look like the Bot user type has made its way into a few different parts of the v3 API. Here's another example where .merged_by.type is Bot. It looks like we're getting lucky on this occasion because SimpleUser doesn't have a type field otherwise this library would fail to decode some pull requests.

Since it looks like the Bot type is here to stay in the v3 API, it seems like a good idea to make this change otherwise we'll eventually end up with users raising issues because they're getting Unknown OwnerType errors.

@@ -205,7 +205,7 @@ parseOrganization obj = Organization
<*> obj .: "created_at"

instance FromJSON User where
parseJSON = mfilter ((== OwnerUser) . userType) . withObject "User" parseUser
parseJSON = withObject "User" parseUser
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still check it's not an OwnerOrganisation.

Copy link
Contributor

@phadej phadej left a comment

Choose a reason for hiding this comment

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

Ok. I think this change is unavoidable. I'll improve the haddocks fo OwnerType after this is merged, yet there's one small issue in the code.

@domenkozar
Copy link
Contributor Author

@phadej fixed

@phadej phadej merged commit 7c0070b into haskell-github:master Sep 29, 2019
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.

None yet

4 participants