Skip to content

Update authentication section for clarity #954

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 4 commits into from
Jun 14, 2018
Merged

Conversation

SirSpidey
Copy link
Contributor

@SirSpidey SirSpidey commented Jun 14, 2018

Addresses changes requested from reviewers to make the authentication section clearer as we migrate services to IAM.

Summary

  • Update intro to authentication
  • Move IAM to top of the stack of auth methods. Rename to just IAM
  • Update getting credentials section
  • Update Visual Recognition note
  • Change headings to sentence case
  • Update toc

cc @mkistler @ehdsouza @anweshan @mediumTaj

- Update intro to authentication
- Move IAM to top of the stack of auth methods. Rename to just `IAM`
- Update getting credentials section
- Update Visual Recognition note
- Change headings to sentence case
- Update toc
@SirSpidey SirSpidey requested a review from lpatino10 June 14, 2018 16:43
@SirSpidey
Copy link
Contributor Author

@lopa, one thing missing from IAM here (and most other SDKs) that's available in the Swift SDK is an example of how to refresh the token. Not sure if that's needed in general

discovery.accessToken("new-accessToken-here")

@codecov-io
Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #954 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #954   +/-   ##
==========================================
  Coverage      39.05%   39.05%           
  Complexity      1470     1470           
==========================================
  Files            580      580           
  Lines          14675    14675           
  Branches         840      840           
==========================================
  Hits            5731     5731           
  Misses          8610     8610           
  Partials         334      334

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f55fac7...641b845. Read the comment docs.

@lpatino10
Copy link
Contributor

What's your opinion on that? We don't offer an exposed method to refresh a user's token, so if their token did expire and they need to use a new one, they would basically need to set the IamOptions again like the first time, now with the valid token.

I think that might be fine to leave out of explicit documentation but I could see the case for it.

@SirSpidey
Copy link
Contributor Author

I don't have a strong opinion -- just noting that Mr. Kistler has it. We do have a link in the IAM section to how the process works. Because we're not supporting it, I think it makes more sense not to include it.

@lopa, I updated the vcap info to point to our docs page. Does that work for the usage here?
Copy link
Contributor

@lpatino10 lpatino10 left a comment

Choose a reason for hiding this comment

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

All looks good to me 👍 Thanks for the updates!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants