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.

Sign up for GitHub

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

Jump to bottom

add cli describer for oauthtokens #9032

Merged
merged 1 commit into from
May 28, 2016
Merged

add cli describer for oauthtokens #9032

merged 1 commit into from
May 28, 2016

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented May 25, 2016 •
edited
Loading

Adds a command line describer for OAuth tokens:
oc describe oauthaccesstoken <token-name>

Name:           <token-name>
Namespace:      <none>
Created:        2 hours ago
Labels:         <none>
Annotations:    <none>
Scopes:         []
Expires In:     24 hours
User Name:      test
User UID:       322b236b-22b9-11e6-b307-080027242396
Refresh Token:  <none>
Client Name:    openshift-challenging-client

fixes #9012

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2016
deads2k
deads2k reviewed May 26, 2016
View reviewed changes
pkg/cmd/cli/describe/describer.go
formatMeta(out, oAuthAccessToken.ObjectMeta)
formatString(out, "Client Name", oAuthAccessToken.ClientName)
formatString(out, "Expires In", oAuthAccessToken.ExpiresIn)
formatString(out, "Redirect URI", oAuthAccessToken.RedirectURI)
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this value is very useful. The redirect already happened and someone doing a post-mortem on a token will have the json.

@deads2k
Copy link
Contributor

deads2k commented May 26, 2016

Looks good, the output just needs a little swizzling.

@juanvallejo juanvallejo force-pushed the jvallejo_add-cli-oauth-describer branch from 9bc0497 to d15b954 Compare May 26, 2016 17:09
@juanvallejo
Copy link
Contributor Author

Updated output:

oc describe oauthaccesstoken <token-name>

Name:           <token-name>
Namespace:      <none>
Created:        2 hours ago
Labels:         <none>
Annotations:    <none>
Scopes:         []
Expires In:     24 hours
User Name:      test
User UID:       322b236b-22b9-11e6-b307-080027242396
Refresh Token:  <none>
Client Name:    openshift-challenging-client

@liggitt
Copy link
Contributor

liggitt commented May 26, 2016

expires in should either be an absolute date, or relative to the current time

@deads2k
Copy link
Contributor

deads2k commented May 26, 2016

expires in should either be an absolute date, or relative to the current time

Do you happen to know the answer to my question; #9032 (comment) ?

@deads2k
Copy link
Contributor

deads2k commented May 26, 2016

@juanvallejo found it . FormatRelativeTime

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2016
@juanvallejo
Copy link
Contributor Author

juanvallejo commented May 26, 2016 •
edited
Loading

Updated expiration time to absolute date (helper functions that we have fail to correctly handle a time difference longer than 24h so not using FormatRelativeTime for now, I will submit another pull request addressing this).

Updated console output:

oc describe oauthaccesstoken <token-name>

Name:           <token-name>
Namespace:      <none>
Created:        6 hours ago
Labels:         <none>
Annotations:    <none>
Scopes:         []
Expires:        May 27, 14:37 +0000 UTC
User Name:      test
User UID:       322b236b-22b9-11e6-b307-080027242396
Refresh Token:  <none>
Client Name:    openshift-challenging-client

@juanvallejo
Copy link
Contributor Author

juanvallejo commented May 27, 2016 •
edited
Loading

@deads2k
Updated console output:

oc describe oauthaccesstoken <token-name>

Name:           <token-name>
Namespace:      <none>
Created:        11 hours ago
Labels:         <none>
Annotations:    <none>
Scopes:         []
Expires In:     12 hours
User Name:      test
User UID:       322b236b-22b9-11e6-b307-080027242396
Refresh Token:  <none>
Client Name:    openshift-challenging-client

deads2k
deads2k reviewed May 27, 2016
View reviewed changes
pkg/cmd/cli/describe/describer.go
formatString(out, "Expires In", formatToHumanDuration(timeExpired.Sub(time.Now())))
formatString(out, "User Name", oAuthAccessToken.UserName)
formatString(out, "User UID", oAuthAccessToken.UserUID)
formatString(out, "Refresh Token", oAuthAccessToken.RefreshToken)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't provide these yet. Remove this line.

@deads2k
Copy link
Contributor

deads2k commented May 27, 2016

Can you show me an example 6 months in the future?

deads2k
deads2k reviewed May 27, 2016
View reviewed changes
pkg/cmd/cli/describe/helpers.go
@@ -118,6 +118,16 @@ var timeNowFn = func() time.Time {
return time.Now()
}

func TimeNowFn() time.Time {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you exposing this?

@juanvallejo
Copy link
Contributor Author

@deads2k Output on token with expiration date 6months in the future:

oc describe oauthaccesstoken DYGZDLucARCPIfUeKPhsgPfn0WBLR_9KdeREH0c9ioc

Name:           DYGZDLucARCPIfUeKPhsgPfn0WBLR_9KdeREH0c9ioc
Namespace:      <none>
Created:        13 minutes ago
Labels:         <none>
Annotations:    <none>
Scopes:         []
Expires In:     5 months
User Name:      test
User UID:       322b236b-22b9-11e6-b307-080027242396
Client Name:    openshift-challenging-client

@deads2k
Copy link
Contributor

deads2k commented May 27, 2016

@deads2k Output on token with expiration date 6months in the future:

Ok, go ahead and squash down

deads2k
deads2k reviewed May 27, 2016
View reviewed changes
test/fixtures/oauthaccesstoken.yaml
creationTimestamp: 2016-05-27T15:06:11Z
deletionTimestamp: 2016-11-28T15:06:11Z
name: DYGZDLucARCPIfUeKPhsgPfn0WBLR_9KdeREH0c9iod
resourceVersion: "5338"
Copy link
Contributor

Choose a reason for hiding this comment

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

remove creation,deleteion,resourceversion,selflink,uid

@juanvallejo juanvallejo force-pushed the jvallejo_add-cli-oauth-describer branch from 0d3243d to a353593 Compare May 27, 2016 20:02
@juanvallejo
Copy link
Contributor Author

@deads2k Okay, revised test/fixtures/oauthaccesstoken.yaml and squashed down

@deads2k
Copy link
Contributor

deads2k commented May 27, 2016

lgtm [merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented May 27, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4176/) (Image: devenv-rhel7_4293)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to a353593

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to a353593

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4167/)

@openshift-bot openshift-bot merged commit de8130a into openshift:master May 28, 2016
@juanvallejo juanvallejo deleted the jvallejo_add-cli-oauth-describer branch May 31, 2016 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@deads2k deads2k

Labels
area/usability component/cli
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build a describer for OAuth access tokens
5 participants
@juanvallejo @deads2k @liggitt @openshift-bot @stevekuznetsov