Skip to content

Github provider #10

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

Merged
merged 9 commits into from
Sep 13, 2014
Merged

Github provider #10

merged 9 commits into from
Sep 13, 2014

Conversation

freiric
Copy link
Contributor

@freiric freiric commented Sep 7, 2014

Hi, I would need the github provider.
I have rebased #5 onto master and done some minor corrections:
I've written a second query to fetch the email, corrected one typo (scope instead of scopes in the initial authentication query), and hardcoded the scope 'user:email'.
(I am not quite sure why #5 was not merged in the first place... )

cpennington and others added 9 commits August 29, 2014 20:57
and avoid authenticate 1.3.2.6 which gave the following strange error:
...
Building authenticate-1.3.2.6...
Preprocessing library authenticate-1.3.2.6...
[1 of 9] Compiling OpenId2.XRDS     ( OpenId2/XRDS.hs, dist/dist-sandbox-a1429708/build/OpenId2/XRDS.o )
[2 of 9] Compiling Web.Authenticate.OpenId.Providers ( Web/Authenticate/OpenId/Providers.hs, dist/dist-sandbox-a1429708/build/Web/Authenticate/OpenId/Providers.o )
[3 of 9] Compiling Web.Authenticate.BrowserId ( Web/Authenticate/BrowserId.hs, dist/dist-sandbox-a1429708/build/Web/Authenticate/BrowserId.o )

Web/Authenticate/BrowserId.hs:15:22:
    Module ‘Data.Conduit’ does not export ‘MonadBaseControl’

Web/Authenticate/BrowserId.hs:15:40:
    Module ‘Data.Conduit’ does not export ‘MonadResource’
Failed to install authenticate-1.3.2.6
…l anyway have to be changed to include more queries)
@scan
Copy link
Contributor

scan commented Sep 7, 2014

Maybe @pbrisbin can take a look at it to tell us if it's okay to merge.

@pbrisbin
Copy link
Member

pbrisbin commented Sep 7, 2014

I had intended to leave #5 to @scan to merge at their discretion, sorry for not making that clearer.

Assuming this good (I haven't reviewed closely yet), how should we proceed? Merged #5, rebase this, then merge this, or abandon #5?

@freiric
Copy link
Contributor Author

freiric commented Sep 7, 2014

I would need to have the email address of the user from github, and 'scopes' do need to be replaced by 'scope' in the authentication query, but I am not sure about the upgrade of authenticate > 1.3.2.6 is really needed (it just did not compiled on my machine).

@pbrisbin
Copy link
Member

I'll be able to look at this on Friday. I'll pull it down, make sure it compiles in a clean sandbox, then release it.

Since @cpennington 's commits are here and correctly attributed, I don't see any reason to merge #5 first.

Thanks!

@scan
Copy link
Contributor

scan commented Sep 12, 2014

I'd like to leave Pull Requests to you for now, @pbrisbin. I'm currently too far out of Haskell and don't have the time to get myself back into it for now. Sorry if it's an inconvenience.

pbrisbin added a commit that referenced this pull request Sep 13, 2014
@pbrisbin pbrisbin merged commit 3b2aeed into freckle:master Sep 13, 2014
@pbrisbin pbrisbin removed their assignment Mar 31, 2015
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.

4 participants