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

Add oc serviceaccounts create-kubeconfig #12082

Merged
merged 2 commits into from
Dec 3, 2016

Conversation

smarterclayton
Copy link
Contributor

No description provided.

@smarterclayton
Copy link
Contributor Author

@fabianofranz

@smarterclayton
Copy link
Contributor Author

@openshift/cli-review

@juanvallejo
Copy link
Contributor

@fabianofranz LGTM

@juanvallejo
Copy link
Contributor

juanvallejo commented Dec 1, 2016

@smarterclayton updated docs look fine, but travis seems to be generating outdated docs when checking
@stevekuznetsov fyi

@stevekuznetsov
Copy link
Contributor

@juanvallejo can you reproduce it ?

@juanvallejo
Copy link
Contributor

@stevekuznetsov

can you reproduce it ?

Yeah, see #12058

@stevekuznetsov
Copy link
Contributor

@juanvallejo The Travis job in this PR is on some old commit, maybe it's correct for the commit it was checking for? Kick it and see if it happens with the current state of this PR.

@smarterclayton
Copy link
Contributor Author

[test]

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Dec 1, 2016 via email


var (
createKubeconfigLong = templates.LongDesc(`
Create a .kubeconfig file that will utilize this service account.
Copy link
Member

Choose a reason for hiding this comment

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

Create where? Might be worth to mention it just gets printed to the output, and add an example that redirects the output to a file.

const (
CreateKubeconfigRecommendedName = "create-kubeconfig"

createKubeconfigShort = `Create a .kubeconfig file for a service account`
Copy link
Member

Choose a reason for hiding this comment

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

In other places (like oc config) we refer to it as "kubeconfig" (without the dot) or "client config file". I'd suggest "kubeconfig", the long desc could add a mention to "client config" (without "file" given this doesn't output to a file by default).

Copy link
Member

Choose a reason for hiding this comment

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

kubeconfig instead of .kubeconfig in this line.

Copy link
Member

Choose a reason for hiding this comment

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

Also I'd suggest to s/Create/Generate

@fabianofranz
Copy link
Member

Worth adding some hack/test-cmd.sh for this.

@smarterclayton smarterclayton force-pushed the create_kubeconfig branch 2 times, most recently from 2125074 to e1c05f4 Compare December 1, 2016 21:05
@smarterclayton
Copy link
Contributor Author

smarterclayton commented Dec 1, 2016

[test] flake "no route to fedora cloud infra" #9379

@smarterclayton
Copy link
Contributor Author

[test] fedora copr

@smarterclayton
Copy link
Contributor Author

Any additional comments?

Copy link
Member

@fabianofranz fabianofranz left a comment

Choose a reason for hiding this comment

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

A couple nits, after that LGTM.

const (
CreateKubeconfigRecommendedName = "create-kubeconfig"

createKubeconfigShort = `Create a .kubeconfig file for a service account`
Copy link
Member

Choose a reason for hiding this comment

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

kubeconfig instead of .kubeconfig in this line.

const (
CreateKubeconfigRecommendedName = "create-kubeconfig"

createKubeconfigShort = `Create a .kubeconfig file for a service account`
Copy link
Member

Choose a reason for hiding this comment

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

Also I'd suggest to s/Create/Generate

createKubeconfigLong = templates.LongDesc(`
Generate a kubeconfig file that will utilize this service account.

The .kubeconfig will reference the service account token and use the current server,
Copy link
Member

Choose a reason for hiding this comment

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

kubeconfig instead of .kubeconfig in this line

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 886dcbe

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11946/) (Base Commit: b20ed8d)

@smarterclayton
Copy link
Contributor Author

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 886dcbe

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 3, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11962/) (Base Commit: ec171a1) (Image: devenv-rhel7_5472)

@openshift-bot openshift-bot merged commit 93271aa into openshift:master Dec 3, 2016
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.

5 participants