Skip to content

oc project on kube #11120

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
Merged
merged 3 commits into from
Sep 30, 2016
Merged

oc project on kube #11120

merged 3 commits into from
Sep 30, 2016

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Sep 27, 2016

Make oc project work on kube, so I can avoid having to use the kubectl config commands.

@fabianofranz
Copy link
Member

@deads2k are you going to close the --token one?

@deads2k
Copy link
Contributor Author

deads2k commented Sep 27, 2016

@deads2k are you going to close the --token one?

I can if you want. I want both commands eventually. Don't really care how.

@fabianofranz
Copy link
Member

@deads2k nah, I'm just asking because the --token commit is here too.

@fabianofranz
Copy link
Member

@openshift/cli-review

@juanvallejo
Copy link
Contributor

Would this partially address #10821?

@deads2k
Copy link
Contributor Author

deads2k commented Sep 27, 2016

Would this partially address #10821?

Yeah.

@deads2k
Copy link
Contributor Author

deads2k commented Sep 27, 2016

Test added.

@stevekuznetsov can we add test/extended/cmd.sh to the conformance bucket

juanvallejo
juanvallejo reviewed Sep 27, 2016
View reviewed changes
pkg/cmd/cli/cmd/login/helpers.go
me, err := client.Users().Get("~")

// if we're talking to kube (or likely talking to kube),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this comment seems a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: this comment seems a bit confusing

rewording suggestion? Removal?

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to me. I'm curious what's confusing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I had read the comment as if the wording inside the parenthesis was redundant, or maybe accidentally left there after rephrasing the entire sentence, but it made more sense after reading this comment. Please ignore my first comment, it's fine the way it is

juanvallejo
juanvallejo reviewed Sep 27, 2016
View reviewed changes
pkg/cmd/cli/cmd/login/loginoptions.go
@@ -270,6 +263,12 @@ func (o *LoginOptions) gatherProjectInfo() error {
}

projectsList, err := oClient.Projects().List(kapi.ListOptions{})
// if we're running on kube (or likely kube), just set it to "default"
if kerrors.IsNotFound(err) {
fmt.Fprintf(o.Out, "No ACL filtered namespaces available. Using \"default\". You switch projects with '%s project <projectname>':\n\n", o.CommandName)
Copy link
Contributor

Choose a reason for hiding this comment

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

"You can switch projects with..."

Copy link
Contributor

Choose a reason for hiding this comment

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

"No ACL filtered namespaces available" - will a user have any idea what that means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"No ACL filtered namespaces available" - will a user have any idea what that means?

I struggled with phrasing. A kube user wouldn't recognized "projects" either. Say nothing about it maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I would remove it if we can't find better wording.

@juanvallejo
Copy link
Contributor

A few nits, but LGTM

@stevekuznetsov
Copy link
Contributor

can we add test/extended/cmd.sh to the conformance bucket

You can append it to the list of tasks in test/extended/conformance.sh, sure, if we agree that the tests in cmd.sh are in that bucket.

@deads2k
Copy link
Contributor Author

deads2k commented Sep 27, 2016

You can append it to the list of tasks in test/extended/conformance.sh, sure, if we agree that the tests in cmd.sh are in that bucket.

@smarterclayton I plan to do this unless you shout.

@smarterclayton
Copy link
Contributor

Hrm - I really do prefer for conformance to be a single suite binary. We've never really formally solved cmd.sh, so I am a tiny bit worried about that.

@deads2k
Copy link
Contributor Author

deads2k commented Sep 27, 2016

Hrm - I really do prefer for conformance to be a single suite binary. We've never really formally solved cmd.sh, so I am a tiny bit worried about that.

I don't care about the name as much as I care about running it in a test job.

@fabianofranz
Copy link
Member

[test]

@stevekuznetsov
Copy link
Contributor

I don't care about the name as much as I care about running it in a test job.

Yeah, I guess conformance as a term has weight re: what we ship to customers... maybe a reconfig of the conformance job should happen?

@deads2k deads2k mentioned this pull request Sep 29, 2016
Closed
@deads2k
Copy link
Contributor Author

deads2k commented Sep 29, 2016

comments addressed. test in test/extended/cmd.sh. I'll leave to someone else to decide how to tie it in, but I suspect we'll want a larger bucket testing commands against kube servers ala test-cmd.sh.

@smarterclayton
Copy link
Contributor

LGTM

@juanvallejo juanvallejo mentioned this pull request Sep 29, 2016
Closed
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to d00a6ef

@openshift-bot
Copy link
Contributor

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

@deads2k
Copy link
Contributor Author

deads2k commented Sep 29, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to d00a6ef

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 30, 2016 •
edited
Loading

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

@openshift-bot openshift-bot merged commit b6ba8ec into openshift:master Sep 30, 2016
@deads2k deads2k deleted the oc-project-on-kube branch February 3, 2017 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncdc ncdc ncdc left review comments

@juanvallejo juanvallejo juanvallejo left review comments

Assignees

@fabianofranz fabianofranz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@deads2k @fabianofranz @juanvallejo @stevekuznetsov @smarterclayton @openshift-bot @ncdc