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

sso quickstart/docs: update configuration variables #226

Merged
merged 1 commit into from
Jul 26, 2019

Conversation

Jusshersmith
Copy link
Contributor

Problem

We’ve made some substantial changes to the configuration variables (largely their names, but also what’s optional) required by SSO. This means that the quickstart and other documentation is outdated.

Solution

This PR pulls our various documentation back in line with the configuration variables we now use. Most changes are applicable only to the ‘auth’ portion of SSO; the ‘proxy’ configuration variables have not yet been fully updated.

@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@c5f729c). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #226   +/-   ##
=========================================
  Coverage          ?   62.25%           
=========================================
  Files             ?       50           
  Lines             ?     4069           
  Branches          ?        0           
=========================================
  Hits              ?     2533           
  Misses            ?     1349           
  Partials          ?      187

@@ -122,7 +122,7 @@ have been filled in, click the "Authorize" button.
To give `sso-auth` permission to access Google Group information for users, the following
environment variables must be set:

- **`GOOGLE_ADMIN_EMAIL`**: An administrative email address on your organization's
- **`PROVIDER_GOOGLE_GOOGLE_IMPERSONATE`**: An administrative email address on your organization's
Copy link
Contributor

Choose a reason for hiding this comment

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

The first GOOGLE here doesn't have to be GOOGLE -- it can be any unique identifier that logical groups a set of provider configs together. This is confusing but maybe figuring out a way to call out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah..Let me note that specifically and try to make that a bit clearer - maybe just using a different unique identifier for the quickstart will help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jphines - update the documentation to hopefully make this clearer

Add the below lines into the `env` file you just created:

PROVIDER_GOOGLE_TYPE=google
PROVIDER_GOOGLE_SLUG=google
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should make the identifier this slug instead of passing it in. Too magical?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea. I don't personally feel like it's needed prior to the release (just in case anyone feels differently), but think it would be good to add!

benjsto
benjsto previously approved these changes Jul 23, 2019
Copy link
Contributor

@benjsto benjsto left a comment

Choose a reason for hiding this comment

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

Just went through this last night with my existing dev instance that was several months outdated. After updating my env and docker-compose.yml following these updated instructions, everything worked as expected using google auth. I don’t have a way to test the Okta portion.

@Jusshersmith Jusshersmith requested review from jphines and benjsto and removed request for benjsto and jphines July 25, 2019 11:54
@Jusshersmith Jusshersmith merged commit e9505f1 into master Jul 26, 2019
@mccutchen mccutchen deleted the jusshersmith-update-docs branch November 4, 2019 17:31
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

3 participants