Improve the general message flow for login #1422

smarterclayton · 2015-03-23T23:32:44Z

fabianofranz · 2015-03-24T02:36:02Z

pkg/cmd/cli/cmd/loginoptions.go

Integration tests use these Gather*Info methods.

Bad integration tests. That's unit territory.

On Mar 23, 2015, at 10:36 PM, Fabiano Franz [email protected] wrote:

In pkg/cmd/cli/cmd/loginoptions.go:

@@ -69,13 +69,13 @@ func (o _LoginOptions) GatherInfo() error {
// present ask for interactive user input. Will also ping the server to make sure we can
// connect to it, and if any problem is found (e.g. certificate issues), ask the user about
// connecting insecurely.
-func (o *LoginOptions) GatherServerInfo() error {
Integration tests use these Gather_Info methods.

—
Reply to this email directly or view it on GitHub.

I agree, we don't use the full GatherInfo test in integration tests at the moment because of an issue with project caching (test assigns a project to user, user's project list does not reflect it right after), so we have to avoid GatherProjectInfo. There's a TODO in test_login to uncomment when the issue is fixed.

----- Original Message -----

@@ -69,13 +69,13 @@ func (o *LoginOptions) GatherInfo() error {
// present ask for interactive user input. Will also ping the server to
make sure we can
// connect to it, and if any problem is found (e.g. certificate issues),
ask the user about
// connecting insecurely.
-func (o *LoginOptions) GatherServerInfo() error {

Bad integration tests. That's unit territory.

On Mar 23, 2015, at 10:36 PM, Fabiano Franz [email protected]
wrote:

In pkg/cmd/cli/cmd/loginoptions.go:

@@ -69,13 +69,13 @@ func (o _LoginOptions) GatherInfo() error {
// present ask for interactive user input. Will also ping the server to
make sure we can
// connect to it, and if any problem is found (e.g. certificate issues),
ask the user about
// connecting insecurely.
-func (o *LoginOptions) GatherServerInfo() error {
Integration tests use these Gather_Info methods.

—
Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1422/files#r27002506

I don't think these should be exposed since they're implementation detail. Agree it's important to test them, but exposing them was making them seem like reusable function and I don't know that we have a use case for that now.

----- Original Message -----

@@ -69,13 +69,13 @@ func (o *LoginOptions) GatherInfo() error {
// present ask for interactive user input. Will also ping the server to
make sure we can
// connect to it, and if any problem is found (e.g. certificate issues),
ask the user about
// connecting insecurely.
-func (o *LoginOptions) GatherServerInfo() error {

I agree, we don't use the full GatherInfo test in integration tests at the
moment because of an issue with project caching (test assigns a project to
user, user's project list does not reflect it right after), so we have to
avoid GatherProjectInfo. There's a TODO in test_login to uncomment when
the issue is fixed.

----- Original Message -----

@@ -69,13 +69,13 @@ func (o *LoginOptions) GatherInfo() error {
// present ask for interactive user input. Will also ping the server to
make sure we can
// connect to it, and if any problem is found (e.g. certificate issues),
ask the user about
// connecting insecurely.
-func (o *LoginOptions) GatherServerInfo() error {

Bad integration tests. That's unit territory.

On Mar 23, 2015, at 10:36 PM, Fabiano Franz [email protected]
wrote:

In pkg/cmd/cli/cmd/loginoptions.go:

@@ -69,13 +69,13 @@ func (o _LoginOptions) GatherInfo() error {
// present ask for interactive user input. Will also ping the server
to
make sure we can
// connect to it, and if any problem is found (e.g. certificate
issues),
ask the user about
// connecting insecurely.
-func (o *LoginOptions) GatherServerInfo() error {
Integration tests use these Gather_Info methods.

—
Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1422/files#r27002506

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1422/files#r27003159

smarterclayton · 2015-03-24T17:06:18Z

@liggitt or @jwforres can you review?

jwforres · 2015-03-24T21:37:32Z

pkg/cmd/cli/cmd/loginoptions.go

If you already have a project set during login then you are probably not a first time user so don't dump the list of projects.

smarterclayton · 2015-03-24T22:11:47Z

[merge] after addressing jessica's comments

openshift-bot · 2015-03-24T22:15:37Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1288/) (Image: devenv-fedora_1127)

openshift-bot · 2015-03-24T22:15:37Z

[Test]ing while waiting on the merge queue

openshift-bot · 2015-03-24T22:20:38Z

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

smarterclayton · 2015-03-25T01:01:27Z

[merge]

smarterclayton · 2015-03-25T04:41:53Z

[merge]

fabianofranz · 2015-03-25T13:49:18Z

[merge] again

openshift-bot · 2015-03-25T13:50:37Z

Evaluated for origin up to e6e99d1

Merged by openshift-bot

…service-catalog/' changes from aa27078754..dabde2eb85 dabde2eb85 origin build: add origin tooling b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459) 4bea012 Use versioned client APIs (openshift#1458) ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452) 3fddf27 clean up logic and fix message for failed poll (openshift#1451) 40926cd Fix typo from openshift#1354 (openshift#1456) ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444) 8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448) ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343) 7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439) baf28de Create listers before adding event handlers in controller (openshift#1446) 294157d remove setServiceBindingCondition dependency on controller (openshift#1441) 118a0f7 Fix typo in validation (openshift#1447) 117bfbd clean up error logging (openshift#1443) dff470f Move "External" around in some resource names/properties (openshift#1354) 0885edb Adding expectedGot function and using it. (openshift#1440) a7d582e Pretty controller broker (openshift#1442) c5edfaf Set apimachinery build variables with semver info (openshift#1429) 0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408) fb874df Remove deprecated basic auth config support (openshift#1431) f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433) 96b286e Make service/plan reference fields on instance spec selectable (openshift#1422) 33f2b04 First example using the pretty context builder. (openshift#1403) 7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417) fcf9480 Add tests for plan updates (openshift#1412) 819332e Add root CAs (openshift#1419) b49a76a Clean Makefile a little (openshift#1399) d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415) 314a622 Wire etcd prefix to storage and call complete with options (openshift#1394) REVERT: aa27078754 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: dabde2eb859b5e31e97c01a704561fc27e1848b2

…service-catalog/' changes from aa27078754..510060232e 510060232e origin build: add origin tooling de45e94 v0.1.0 chart changes (openshift#1468) 0bb9982 Modify Makefile to only specify ldflags once (openshift#1471) 5d6afac Fixes openshift#735: Add repo-sync script for charts (openshift#1453) 630f13f fix lingering unversioned client API (openshift#1466) 6f49128 Fix several logging errors (openshift#1464) 2aece61 Delete removed serviceClasses when they have no instances left (openshift#1450) 179d302 Uncommenting UID field after updating to k8s 1.8 (openshift#1457) b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459) 4bea012 Use versioned client APIs (openshift#1458) ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452) 3fddf27 clean up logic and fix message for failed poll (openshift#1451) 40926cd Fix typo from openshift#1354 (openshift#1456) ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444) 8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448) ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343) 7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439) baf28de Create listers before adding event handlers in controller (openshift#1446) 294157d remove setServiceBindingCondition dependency on controller (openshift#1441) 118a0f7 Fix typo in validation (openshift#1447) 117bfbd clean up error logging (openshift#1443) dff470f Move "External" around in some resource names/properties (openshift#1354) 0885edb Adding expectedGot function and using it. (openshift#1440) a7d582e Pretty controller broker (openshift#1442) c5edfaf Set apimachinery build variables with semver info (openshift#1429) 0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408) fb874df Remove deprecated basic auth config support (openshift#1431) f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433) 96b286e Make service/plan reference fields on instance spec selectable (openshift#1422) 33f2b04 First example using the pretty context builder. (openshift#1403) 7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417) fcf9480 Add tests for plan updates (openshift#1412) 819332e Add root CAs (openshift#1419) b49a76a Clean Makefile a little (openshift#1399) d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415) 314a622 Wire etcd prefix to storage and call complete with options (openshift#1394) REVERT: aa27078754 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 510060232e54eb64b294213bb5d7847e169a2fac

…hift#1422)

fabianofranz reviewed Mar 24, 2015
View reviewed changes

smarterclayton force-pushed the fix_login_messages branch from fa02962 to be98f83 Compare March 24, 2015 17:06

jwforres reviewed Mar 24, 2015
View reviewed changes

Improve the general message flow for login

e6e99d1

smarterclayton force-pushed the fix_login_messages branch from be98f83 to e6e99d1 Compare March 24, 2015 22:11

openshift-bot pushed a commit that referenced this pull request Mar 25, 2015

Merge pull request #1422 from smarterclayton/fix_login_messages

7abb512

Merged by openshift-bot

openshift-bot merged commit 7abb512 into openshift:master Mar 25, 2015

smarterclayton deleted the fix_login_messages branch May 18, 2015 02:16

jpeeler pushed a commit to jpeeler/origin that referenced this pull request Feb 1, 2018

Make service/plan reference fields on instance spec selectable (opens…

96b286e

…hift#1422)

Improve the general message flow for login #1422

Improve the general message flow for login #1422

Uh oh!

Conversation

smarterclayton commented Mar 23, 2015

Uh oh!

fabianofranz Mar 24, 2015

Choose a reason for hiding this comment

Uh oh!

smarterclayton Mar 24, 2015

Choose a reason for hiding this comment

Uh oh!

fabianofranz Mar 24, 2015

Choose a reason for hiding this comment

Uh oh!

smarterclayton Mar 24, 2015

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Mar 24, 2015

Uh oh!

jwforres Mar 24, 2015

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Mar 24, 2015

Uh oh!

openshift-bot commented Mar 24, 2015

Uh oh!

openshift-bot commented Mar 24, 2015

Uh oh!

openshift-bot commented Mar 24, 2015

Uh oh!

smarterclayton commented Mar 25, 2015

Uh oh!

smarterclayton commented Mar 25, 2015

Uh oh!

fabianofranz commented Mar 25, 2015

Uh oh!

openshift-bot commented Mar 25, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants