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 keycloak provider #227

Merged
merged 9 commits into from
Sep 25, 2019
Merged

Conversation

Ofinka
Copy link

@Ofinka Ofinka commented Jul 28, 2019

Adding integration of keycloak provider based on forgotten PR https://github.com/bitly/oauth2_proxy/pull/366/files

Solves #127

How Has This Been Tested?

Tested manually. I set up keycloak and reference it in configuration of oauth2_proxy using this provider.

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@Ofinka Ofinka requested a review from a team July 28, 2019 14:04
@mannp
Copy link

mannp commented Jul 28, 2019

Excellent, thank you for taking the time to do this :-)

@loshz
Copy link
Contributor

loshz commented Jul 30, 2019

LGTM 👍 - @JoelSpeed do we have any additional processes that need completing when adding a new provider?

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

I've typically asked people to become maintainers of the provider and add themselves for the new files to the owners file, though I'm happy to let this one slide as it's pretty simple (and is mainly just the group email fetching code)

Need one small change then we can merge this

options.go Outdated Show resolved Hide resolved
loshz
loshz previously approved these changes Jul 31, 2019
Copy link
Contributor

@loshz loshz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@steakunderscore
Copy link
Contributor

@Ofinka Do you mind adding yourself to the owners file for future changes? https://github.com/pusher/oauth2_proxy/blob/master/.github/CODEOWNERS#L12

@mannp
Copy link

mannp commented Aug 17, 2019

Has this one stalled now, as it seemed to be close to getting merged 😃

@Ofinka
Copy link
Author

Ofinka commented Aug 19, 2019

Hi guys, I'm sorry but I'm afraid I do not have enough time to be part of another project :(

Would definitely appreciate if anyone else could take the responsibility.

@steakunderscore
Copy link
Contributor

Understood. I think we should merge this as is.

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Hi guys, I'm sorry but I'm afraid I do not have enough time to be part of another project :(

I appreciate this...

If you can fix up the extra lines added to the changelog then I'm happy to merge as is, thanks! 🙂

CHANGELOG.md Outdated Show resolved Hide resolved
@mleklund
Copy link

Would love to see this move along.

@loshz
Copy link
Contributor

loshz commented Sep 25, 2019

If we can fix @JoelSpeed's comment above, then I can get this merged.

@mleklund
Copy link

The author of the pull request has not responded since August 19, not sure when or if they will.

@loshz
Copy link
Contributor

loshz commented Sep 25, 2019

I have fixed the CHANGELOG error. Just need to get it re-reviewed by @JoelSpeed or @steakunderscore

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Ok, LGTM

@JoelSpeed JoelSpeed merged commit 44cdcc7 into oauth2-proxy:master Sep 25, 2019
@loshz
Copy link
Contributor

loshz commented Sep 25, 2019

Thank you and congrats on your first contribution! 🎉

@mleklund
Copy link

Thanks for your help!!

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.

7 participants