feat: sc-240964/fetch token#326
Conversation
dbolson
force-pushed
the
sc-240964/fetch-token
branch
from
247ce7e to
d46bc6f
Compare
dbolson
force-pushed
the
sc-240964/fetch-token
branch
from
d46bc6f to
19715a8
Compare
| ) | ||
| if err != nil { | ||
| return output.NewCmdOutputError(err, viper.GetString(cliflags.OutputFlag)) | ||
| return err |
There was a problem hiding this comment.
This is only run by a user, so we don't need to return anything but plain text.
| return err | ||
| } | ||
|
|
||
| fmt.Fprintf(cmd.OutOrStdout(), "Your token is %s\n", deviceAuthorizationToken.AccessToken) |
There was a problem hiding this comment.
Another PR will set this value in the config instead of showing it.
| maxAttempts int, | ||
| ) (DeviceAuthorizationToken, error) { | ||
| var attempts int | ||
| for { |
There was a problem hiding this comment.
This can block since we're not doing anything else while the user logs into LD.
| case "authorization_pending": | ||
| attempts += 1 | ||
| case "access_denied": | ||
| return DeviceAuthorizationToken{}, errors.NewError("Your request has been denied. Please try logging in again.") |
There was a problem hiding this comment.
I don't have a strong opinion on these messages if you think of something better.
There was a problem hiding this comment.
also no strong opinion.
in the happy path, the individual denying the request is the same as the individual running this CLI command, which means please try logging in again may be presumptuous of their requirements.
| deviceCode, | ||
| ) | ||
| res, err := client.MakeRequest("POST", path, []byte(body)) | ||
| body, _ := json.Marshal(map[string]string{ |
There was a problem hiding this comment.
Marshal this instead of using a string since the test mock complains if the whitespace differs from the test setup. Marshaling removes extra whitespace to match the tests
There was a problem hiding this comment.
works for me. you could also remove all whitespace at each location.
| var attempts int | ||
| for { | ||
| if attempts > maxAttempts { | ||
| return DeviceAuthorizationToken{}, errors.NewError("The request timed-out after too many attempts.") |
There was a problem hiding this comment.
i think i usually see it spelled timed out
| case "authorization_pending": | ||
| attempts += 1 | ||
| case "access_denied": | ||
| return DeviceAuthorizationToken{}, errors.NewError("Your request has been denied. Please try logging in again.") |
There was a problem hiding this comment.
also no strong opinion.
in the happy path, the individual denying the request is the same as the individual running this CLI command, which means please try logging in again may be presumptuous of their requirements.
| deviceCode, | ||
| ) | ||
| res, err := client.MakeRequest("POST", path, []byte(body)) | ||
| body, _ := json.Marshal(map[string]string{ |
There was a problem hiding this comment.
works for me. you could also remove all whitespace at each location.
| mockClient.On( | ||
| "MakeRequest", | ||
| "POST", | ||
| "http://test.com/internal/device-authorization/token", |
There was a problem hiding this comment.
i like example.com or example.org since there are reserved by ICANN and guaranteed to never be real webapps. maybe test.com is similar? i doubt it matters.
2 participants
Attempt to fetch an access token from the
POST /tokensendpoint. It retries every second if the request is pending up to two minutes while the user logs in to LD to approve their request. Other errors show a specific error message so the user knows what to do.Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.