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

Added 'no projects and cant create' empty states #2296

Merged
merged 1 commit into from
Oct 25, 2017

Conversation

dtaylor113
Copy link
Contributor

@dtaylor113 dtaylor113 commented Oct 18, 2017

Added to deploy-image, from-file, and process-template:

image

image

image

@dtaylor113 dtaylor113 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2017
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 18, 2017
@dtaylor113
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2017
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 20, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2017
@dtaylor113 dtaylor113 changed the title [WIP] Added 'no projects and cant create' empty states Added 'no projects and cant create' empty states Oct 20, 2017
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2017
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2017
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

LGTM except for the one comment. PR will need rebase

$scope.$on('$destroy', hideErrorNotifications);
$scope.$on('$destroy', function() {
hideErrorNotifications();
noProjectsCantCreateWatch();
Copy link
Member

@spadgett spadgett Oct 20, 2017

Choose a reason for hiding this comment

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

@dtaylor113 This is fine, but not needed. The $scope listeners are already deregistered on destroy. I'd honestly rather remove this logic from the controllers to keep the code simpler. It can just be...

$scope.$on('no-projects-cannot-create', function() {
  $scope.noProjectsCantCreate = true;
});

in each of the controllers without needing to handle the return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fixed

@spadgett spadgett self-assigned this Oct 20, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2017
@spadgett
Copy link
Member

/retest

@spadgett
Copy link
Member

Thanks @dtaylor113

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2017
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2017
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 23, 2017
@openshift-merge-robot openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 23, 2017
@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2017
@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2017
@spadgett
Copy link
Member

/retest

@spadgett
Copy link
Member

@dtaylor113 Can you take a look at the test failures and make sure they're not due to your changes?

@dtaylor113
Copy link
Contributor Author

@dtaylor113 Can you take a look at the test failures and make sure they're not due to your changes?

Looking at the Jenkins logs, seems like an issue with 4 or 5 e2e tests. At the moment I have to clear my docker cache and get oc to cluster up. I'll let you know if e2e works locally for me.

@dtaylor113
Copy link
Contributor Author

Hi @spadgett, when I run the e2e tests locally I error out with:

Error: ECONNREFUSED connect ECONNREFUSED 10.193.20.254:43082
    at ClientRequest.<anonymous> (/home/dtaylor/Dev/repos/openshift/origin-web-console/node_modules/selenium-webdriver/http/index.js:145:16)
    at emitOne (events.js:96:13)
    at ClientRequest.emit (events.js:188:7)
    at Socket.socketErrorListener (_http_client.js:308:9)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at emitErrorNT (net.js:1271:8)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)
==== async task ====
WebDriver.takeScreenshot()

I think this is just a general error trying to run selenium on my linux system.

Here is a link to the e2e test failures of this PR:
https://gist.github.com/dtaylor113/5049e774e24066cdbb5d288269d3c0e3

They don't appear to be related to the <select project> changes done in this PR.

@spadgett
Copy link
Member

We keep seeing the same failures in your PR and not others. Those pages also use the select-project component, so it might be related.

/retest

@dtaylor113
Copy link
Contributor Author

Hmm... @juanvallejo is getting the same stack trace reported here on his linux system, latest master. I also am seeing failures on my system using the latest master.

@spadgett
Copy link
Member

@dtaylor113 But your failures are different than Jenkins, right?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2017
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 25, 2017
@openshift-merge-robot openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 25, 2017
</a>
</div>
</form>
<span ng-if="!noProjectsCantCreate">
Copy link
Member

Choose a reason for hiding this comment

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

It might be safer to change these ng-if to ng-show (here and below) to avoid problems like openshift/origin-web-catalog#526

It might fix the test failures as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, replaced ng-if's with ng-shows's, e2e worked with Ben's latest e2e changes and seemed to get further just in this PRs branch. 🤞 Hopefully it'll pass Travis :-)

@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017
@dtaylor113
Copy link
Contributor Author

yay!!! \0/

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2017
@openshift-merge-robot openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 25, 2017
@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 798218c into openshift:master Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants