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

Easier configuration for Basics of Authentication "Good" example #81

Closed

Conversation

@jgkite
Copy link
Contributor

jgkite commented Dec 7, 2015

I ran into a few issues using the Basics of Authentication sample app (advanced_server.rb):

  1. The GitHub URL wasn't configurable. This is generally okay, but I needed to point the sample app at a different :octocat: URL for testing.
  2. I didn't like having to remember environment variables. 😁
  3. Missing or mis-typed configuration options didn't fail loudly, and I usually found out about them by clicking around in the browser getting confusing error messages.

This PR introduces STDIN fallbacks for missing configuration. It also elevates the GitHub URL to a configuration option (defaulting to https://github.com instead of hardcoding in various places. The API base URL is set programmatically based on the :octocat: URL (please check my logic here).


Happy-path usage:

ruby advanced_server.rb
Please enter a value for Client ID: 123
Please enter a value for Client Secret: 456
Please enter a value for Base GitHub URL or, just hit Enter to stick with the default (https://github.com):
<--------------------------------------------------------------------->
GitHub lives here: https://github.com
The GitHub API lives here: https://api.github.com
<--------------------------------------------------------------------->
[2015-12-07 16:16:42] INFO  WEBrick 1.3.1
[2015-12-07 16:16:42] INFO  ruby 2.1.5 (2014-11-13) [x86_64-darwin14.0]
== Sinatra/1.3.5 has taken the stage on 4567 for development with backup from WEBrick
[2015-12-07 16:16:42] INFO  WEBrick::HTTPServer#start: pid=57700 port=4567

Non-interactive usage:

GITHUB_URL=https://github.com \
  GITHUB_CLIENT_ID=123 \
  GITHUB_CLIENT_SECRET=456 \
  ruby advanced_server.rb
<--------------------------------------------------------------------->
GitHub lives here: https://github.com
The GitHub API lives here: https://api.github.com
<--------------------------------------------------------------------->
[2015-12-08 09:43:22] INFO  WEBrick 1.3.1
[2015-12-08 09:43:22] INFO  ruby 2.1.5 (2014-11-13) [x86_64-darwin14.0]
== Sinatra/1.3.5 has taken the stage on 4567 for development with backup from WEBrick
[2015-12-08 09:43:22] INFO  WEBrick::HTTPServer#start: pid=63796 port=4567

Specifying an alternate GitHub URL:

ruby advanced_server.rb
Please enter a value for Client ID: 123
Please enter a value for Client Secret: 456
Please enter a value for Base GitHub URL or, just hit Enter to stick with the default (https://github.com): https://ghe.acme.co
<--------------------------------------------------------------------->
GitHub lives here: https://ghe.acme.co
The GitHub API lives here: https://ghe.acme.co/api/v3
<--------------------------------------------------------------------->

Omitting a required option:

ruby advanced_server.rb
Please enter a value for Client ID:
Sorry, Client ID is required.
@jasonrudolph
Copy link
Member

jasonrudolph commented Dec 8, 2015

@gjtorikian: Would you mind reviewing this when you have a moment?

@gjtorikian
Copy link
Contributor

gjtorikian commented Dec 10, 2015

The overall changes look good, but my concern is that this sample (along with others in the directory) are actually tied to a document that readers should follow as if they were building out their own app. In theory, the documentation and this repo are meant to be more basic proof of concept for using the API. (I say "in theory" because we've never actually discussed what the relationship between this repo and the Dev site content should be.)

I guess my two immediate thoughts are:

  • Can we drop CLIHelper in service of simplifying the process of teaching someone about environment variables and servers?
  • Could a PR also be made to the content to reflect the changes in code here?
@jgkite jgkite self-assigned this Dec 11, 2015
@ryuzaki0

This comment has been minimized.

Leave line note

This comment has been minimized.

Copy link

ryuzaki0 replied Jan 4, 2016

Leave line note

This comment has been minimized.

Copy link

ryuzaki0 replied Jan 4, 2016

Leave line note

This comment has been minimized.

Copy link

ryuzaki0 replied Jan 4, 2016

localhost:4567

@ryuzaki0

This comment has been minimized.

Copy link

ryuzaki0 commented on 38679ad Jan 4, 2016

api/ruby/basics-of-authentication/README.md

This comment has been minimized.

Copy link

ryuzaki0 replied Jan 4, 2016

api/ruby/basics-of-authentication/README.md

@ryuzaki0

This comment has been minimized.

api/ruby/basics-of-authentication/README.md

This comment has been minimized.

Copy link

ryuzaki0 replied Jan 4, 2016

api/ruby/basics-of-authentication/README.md

This comment has been minimized.

Copy link

ryuzaki0 replied Jan 4, 2016

api/ruby/basics-of-authentication/README.md

@ryuzaki0

This comment has been minimized.

api/ruby/basics-of-authentication/README.md

@ryuzaki0

This comment has been minimized.

localhost:4567

@ryuzaki0

This comment has been minimized.

api/ruby/basics-of-authentication/README.md

Copy link

jeeva2530 left a comment

Pls active my application

@github github locked and limited conversation to collaborators Nov 16, 2017
@jonico jonico closed this Mar 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.