Frontend for namespace selection

[bryk]

1. Sorry for the size, but this PR was all-in or nothing
2. The mechanics for namespace is that user selects it via the dropdown
   and the selection goes through all pages, but has effect on display
   only on list pages.
3. The selector is now ugly. I'll style and improve wording later.

---

This change is

[bryk]

@taimir Can you review this?

[taimir]

OK, let's see.

[codecov-io]

Current coverage is 95.03%

Merging #782 into master will increase coverage by 0.57%

1. File ...ereader_directive.js (not in diff) was modified. more
   • Misses -6
   • Partials 0
   • Hits +6

@@             master       #782   diff @@
==========================================
  Files           194        197     +3   
  Lines          1516       1591    +75   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1432       1512    +80   
+ Misses           84         79     -5   
  Partials          0          0          

Powered by Codecov. Last updated by 07ace54...69ae728

[taimir]

Here are a few initial comments and questions. I'll have more time to review this again tomorrow evening and on Wednesday.

Previously, codecov-io wrote…

[Current coverage][cc-pull] is 95.46%

Merging [#782][cc-pull] into [master][cc-base-branch] will increase coverage by 0.68%

1. File ...ereader_directive.js (not in diff) was modified. more
   • Misses -6
   • Partials 0
   • Hits +6

@@             master       #782   diff @@
==========================================
  Files           177        182     +5   
  Lines          1396       1476    +80   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1323       1409    +86   
+ Misses           73         67     -6   
  Partials          0          0          

Powered by Codecov. Last updated by [6cbbb2a...210c9cd][cc-compare]
[cc-base-branch]: https://codecov.io/gh/kubernetes/dashboard/branch/master?src=pr
[cc-compare]: https://codecov.io/gh/kubernetes/dashboard/compare/6cbbb2a214344cd6b97f2c7a3c9a968503148bcb...210c9cdec3ec321cec4b815a56df6f48ab8de3f9
[cc-pull]: https://codecov.io/gh/kubernetes/dashboard/pull/782?src=pr

---

Reviewed 3 of 57 files at r1.
Review status: 3 of 57 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/app/frontend/chrome/chrome_module.js, line 27 [r1] (raw file):

        [
          'ngMaterial',
          'ngResource',

A silly question: namespaceModule already has ngResource as a dependency, so why declare it here again? Is this a "best practice", or am I missing something about angular?

---

src/app/frontend/common/namespace/namespaceselect.html, line 5 [r1] (raw file):

      md-on-close="$ctrl.changeNamespace()">
    <md-option ng-value="namespace" ng-repeat="namespace in $ctrl.namespaces">
      {{$ctrl.formatNamespace(namespace)}}

wouldn't a filter be better suited here?

---

src/app/frontend/common/namespace/namespaceselect_component.js, line 76 [r1] (raw file):

            this.changeNamespace();
          } else {
            this.namespaces = [newNamespace];

O.K, the only case where this if block is executed is when the page is newly loaded (e.g. refresh) and the <kd-namespace-select> has not been clicked yet to get the namespaces from the backend. Is the point of this to preserve the namespace that was selected before the refresh?

I'm asking because when I type in some random namespace manually, e.g. ?namespace=whatever before I've clicked the selection box, the state does indeed change to this non-existing namespace. So the question is this the expected behavior? It kind of lets the user to manually navigate to namespaces that are will not be listed by the backend, which contradicts the check in the previous block.

---

src/app/frontend/common/namespace/namespaceselect_component.js, line 92 [r1] (raw file):

  formatNamespace(namespace) {
    if (namespace === NAMESPACE_NOT_SELECTED) {
      return 'namespace not selected';

Please introduce a getMsg() variable for this.

---

src/app/frontend/common/namespace/namespaceselect_component.js, line 107 [r1] (raw file):

    }
    this.state_.go('.', {[namespaceParam]: namespaceToGo});
  }

Hmm, why not

if (this.selectedNamespace !== NAMESPACE_NOT_SELECTED) {
    this.state_.go('.', {[namespaceParam]: this.selectedNamespace});
}

It's saves up one state change and a couple of lines.

---

Comments from Reviewable

[bryk]

PTAL

Previously, taimir (Atanas Mirchev) wrote…

Here are a few initial comments and questions. I'll have more time to review this again tomorrow evening and on Wednesday.

---

Review status: 3 of 59 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/app/frontend/chrome/chrome_module.js, line 27 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

A silly question: namespaceModule already has ngResource as a dependency, so why declare it here again? Is this a "best practice", or am I missing something about angular?

Modules should declare all direct dependencies. Otherwise, when namespaceModule removes its dep to ngResource, this will break. Think about, e.g., Java where you must import packages you use, no matter that they are transitively included.

---

src/app/frontend/common/namespace/namespaceselect.html, line 5 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

wouldn't a filter be better suited here?

Yeah, it could be here. But this would be an overkill since namespace formatting is only used here and is implementation detail of the component. That's why I opted for format function.

What do you think?

---

src/app/frontend/common/namespace/namespaceselect_component.js, line 76 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

O.K, the only case where this if block is executed is when the page is newly loaded (e.g. refresh) and the <kd-namespace-select> has not been clicked yet to get the namespaces from the backend. Is the point of this to preserve the namespace that was selected before the refresh?

I'm asking because when I type in some random namespace manually, e.g. ?namespace=whatever before I've clicked the selection box, the state does indeed change to this non-existing namespace. So the question is this the expected behavior? It kind of lets the user to manually navigate to namespaces that are will not be listed by the backend, which contradicts the check in the previous block.

That's correct. I implemented this behavior intentionally. It allows to bypass namespaces that do not exist, but avoids downloading all namespaces when not necessary.

What we can do is to validate the namespace here (ask the server whether is extist) and do something when the namespace is invalid. Can we do this in a separate PR?

---

src/app/frontend/common/namespace/namespaceselect_component.js, line 107 [r1] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Hmm, why not

if (this.selectedNamespace !== NAMESPACE_NOT_SELECTED) {
    this.state_.go('.', {[namespaceParam]: this.selectedNamespace});
}

It's saves up one state change and a couple of lines.

But we need to change to NAMESPACE_NOT_SELECTED also. That's why this is required. Try to selecting "no namespace" from the dropdown.

---

Comments from Reviewable

[taimir]

Review status: 3 of 59 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/app/frontend/chrome/chrome_module.js, line 27 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Modules should declare all direct dependencies. Otherwise, when namespaceModule removes its dep to ngResource, this will break. Think about, e.g., Java where you must import packages you use, no matter that they are transitively included.

Not sure I understand your reasoning, especially the java example.

First of all, in the code for the chrome module (everything in the src/app/frontend/chrome dir) I do not see any usage of ngResource. So ngResource is not a direct dependency of the chrome module itself, but a dependency of namespaceModule (which itself is dep of chrome), correct?

Now here's an example applicable to any regular programming lang. (Golang, Java):
Suppose you have a class example.Encryption, which uses the strategy pattern to select a different encryption algorithm. Let algorithms.AESEncryption be one of the algorithm classes, and suppose that this algorithm has a dependencyencryption.Salt.

Now obviously, I'll have to import encryption.Salt in algorithms.AESEncryption, as it is used for its internal logic. I'll also have to import algorithms.AESEncryption in example.Encryption to realize the strategy pattern. But I will not import encryption.Salt in example.Encryption - the internal implementation of algorithms.AESEncryption should remain concealed, to preserve the encapsulation. example.Encryption has no direct dependency on encryption.salt.

Can you clarify how this differs from the angular scenario above? In my mind, example.Encryption is chrome, algorithms.AESEncryption is namespaceModule, and encryption.Salt would be ngResource.

Thanks!

---

Comments from Reviewable

[bryk]

Review status: 2 of 59 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/app/frontend/chrome/chrome_module.js, line 27 [r1] (raw file):

First of all, in the code for the chrome module (everything in the src/app/frontend/chrome dir) I do not see any usage of ngResource. So ngResource is not a direct dependency of the chrome module itself, but a dependency of namespaceModule (which itself is dep of chrome), correct?

You're correct. This should be removed. I made a mistake thinking that this is namespaceModule (which uses ngResource). Thanks for digging this problem!

Removed. PTAL :)

---

Comments from Reviewable

[taimir]

Reviewed 6 of 57 files at r1, 3 of 3 files at r2, 1 of 1 files at r3.
Review status: 12 of 59 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

src/app/frontend/common/namespace/namespace_stateconfig.js, line 26 [r3] (raw file):

    url: `?${namespaceParam}`,
    abstract: true,
    template: '<ui-view></ui-view>',

Hehe, I like this hack. So basically now namespace is an abstract state with no path, but with one parameter ?namespace= that will always be accepted (since namespace is above all).

And namespace's <ui-view></ui-view> is nested in the very first <ui-view></ui-view> in the chrome page where the content was before. That way other states (which are all children of namespace) will be able to insert there content there. So basically it will look like:

<chrome>
    ...
   <!-- the original one-->
   <kd-content ui-view> 
        <!-- the one produced by the namespace state -->
        <ui-view> 
              <!-- e.g. the content of the workloads state-->
              <stuff></stuff> 
        </ui-view>
    </ui-view>
</chrome>

Correct me if I'm wrong, because I need my understanding to be right in order to review this properly.

I like this approach, as it seems really smooth. I have some remarks however, see below.

---

src/app/frontend/common/namespace/namespace_stateconfig.js, line 37 [r3] (raw file):

 * @return {!ui.router.$state}
 */
function requireParentState(stateExtend, parentFn) {

So ok, now every state in dashboard necessarily has namespace as parent. And this is enforced. I also saw that you corrected the breadcrumbs to use only the data.parent of each state, because now the actual parent is always enforced to be namespace. That's fine.

I'm just asking myself: by enforcing this we kind of forbid the use of any nested states in dashboard, because we redirect the parent to namespace always. Is this intended?

My intuition would be to nest everything like "matryoshka dolls", and the biggest one would be namespace. Right now all the smaller dolls are separated and put in the biggest doll (namespace) next to each other, not in each other as they normally would be.

Do you get my concern?

---

src/app/frontend/common/namespace/namespaceselect.html, line 5 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Yeah, it could be here. But this would be an overkill since namespace formatting is only used here and is implementation detail of the component. That's why I opted for format function.

What do you think?

Oh well, you're actually probably right. This use case is way too specific for a filter.

---

src/app/frontend/common/namespace/namespaceselect_component.js, line 76 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

That's correct. I implemented this behavior intentionally. It allows to bypass namespaces that do not exist, but avoids downloading all namespaces when not necessary.

What we can do is to validate the namespace here (ask the server whether is extist) and do something when the namespace is invalid. Can we do this in a separate PR?

Sounds good to me.

---

src/app/frontend/common/namespace/namespaceselect_component.js, line 107 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

But we need to change to NAMESPACE_NOT_SELECTED also. That's why this is required. Try to selecting "no namespace" from the dropdown.

Yes, sorry. I got confused because on the `workloads` state when you change to `no namespace` from `default` nothing happens, so I wrongly assumed that the "go to empty namespace part" is supposed to do nothing.

The only question is, do we need the __NAMESPACE_NOT_SELECTED__?
If all views go to the default namespace when no namespace is selected, that is.

---

src/app/frontend/common/resource/resourcedetail.js, line 36 [r3] (raw file):

export function appendDetailParamsToUrl(baseUrl) {
  return `${baseUrl}/:objectNamespace/:objectName`;

First, I like the idea of having gloabl "resource" template like this :)

One thing I want to comment on here: with the current way the parameters for a resource are passed (objectNamespace and objectName), they become part of the URL path.

When I'm on the detail page of, say, a pod in namespapce default the url is like /pod/default/pod-whatever?namespace=default. Now suppose I switch the namespace from the toolbar to namespace my-namepsace. The URL becomes /pod/default/pod-whatever?namespace=my-namespace ... And now it is totally unclear to me what the user might think. Because he just switched to a different namespace, but still sees the pod from the previous one. I'd like to avoid the discrepancy in the URL between the two namespace params.

One quick solution I can think of is to simply not allow a namespace change on the details view of any resource? Because right now changing the namespace on a details view just does nothing for me.

---

Comments from Reviewable

[taimir]

A few more comments, PTAL.

Previously, bryk (Piotr Bryk) wrote…

PTAL

---

Review status: 12 of 59 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[taimir]

BTW, where is the deploy button (the + on the action bar)? I can't deploy anything now... :(

Previously, taimir (Atanas Mirchev) wrote…

A few more comments, PTAL.

---

Reviewed 35 of 57 files at r1.
Review status: 47 of 59 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

src/app/frontend/logs/logs_stateconfig.js, line 19 [r3] (raw file):

import LogsToolbarController from './logstoolbar/logstoolbar_controller';
import {toolbarViewName} from '../chrome/chrome_state';
import {stateName as namespaceStateName} from 'common/namespace/namespace_state';

Just in the line of my previous comment: does it make sense from a user perspective to change the namespace on the logs page? Given that the rcNamespace and podId are already fixed in the URL and a namespace change in the toolbar won't affect anything.

---

src/app/frontend/replicasetdetail/replicasetdetail_module.js, line 35 [r3] (raw file):

          'ui.router',
          componentsModule.name,
          namespaceModule.name,

O.K., the dependencies on namespaceModule in the other modules seem a bit inconsistent. First I thought that you added a dep to namespaceModule only in the resource list modules (e.g. replicasetlist, podlist, etc.). But now I see this dep in a resource detail module. E.g. there's no dependecy on namespaceModule in the poddetail_module.js.

Is there a reason for this?

---

Comments from Reviewable

[bryk]

PTAL

Previously, taimir (Atanas Mirchev) wrote…

BTW, where is the deploy button (the + on the action bar)? I can't deploy anything now... :(

---

Review status: 47 of 59 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

src/app/frontend/common/namespace/namespace_stateconfig.js, line 26 [r3] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Hehe, I like this hack. So basically now namespace is an abstract state with no path, but with one parameter ?namespace= that will always be accepted (since namespace is above all).

And namespace's <ui-view></ui-view> is nested in the very first <ui-view></ui-view> in the chrome page where the content was before. That way other states (which are all children of namespace) will be able to insert there content there. So basically it will look like:

<chrome>
    ...
   <!-- the original one-->
   <kd-content ui-view> 
        <!-- the one produced by the namespace state -->
        <ui-view> 
              <!-- e.g. the content of the workloads state-->
              <stuff></stuff> 
        </ui-view>
    </ui-view>
</chrome>

Correct me if I'm wrong, because I need my understanding to be right in order to review this properly.

I like this approach, as it seems really smooth. I have some remarks however, see below.

This is 100% correct :) I'll steal this explanation and insert it as a comment in the code so that future devs know what is happening here.

---

src/app/frontend/common/namespace/namespace_stateconfig.js, line 37 [r3] (raw file):

I'm just asking myself: by enforcing this we kind of forbid the use of any nested states in dashboard, because we redirect the parent to namespace always. Is this intended?

No. Of course, you're correct. My intention was to allow for namespace nesting. The only thing I require is to have namespace in the root of state tree.

Now namespace must be the root in namespace tree. So we can have namespace_state <- workloads <- pods namespace tree.

---

src/app/frontend/common/namespace/namespaceselect_component.js, line 107 [r1] (raw file):

The only question is, do we need the NAMESPACE_NOT_SELECTED?

This is needed to have correct label in the UI when "no namespace" is selected. Of course, this could be done in other ways, e.g., we could have compound objects as select models (like, {displayString: foo, namespace: bar}), but both solutions are equivalent.

---

src/app/frontend/common/resource/resourcedetail.js, line 36 [r3] (raw file):

When I'm on the detail page of, say, a pod in namespapce default the url is like /pod/default/pod-whatever?namespace=default. Now suppose I switch the namespace from the toolbar to namespace my-namepsace. The URL becomes /pod/default/pod-whatever?namespace=my-namespace ... And now it is totally unclear to me what the user might think. Because he just switched to a different namespace, but still sees the pod from the previous one. I'd like to avoid the discrepancy in the URL between the two namespace params.

One quick solution I can think of is to simply not allow a namespace change on the details view of any resource? Because right now changing the namespace on a details view just does nothing for me.

Yeah, I'm aware of this issue. I didn't address this in current PR, but my plan is: when you change namespace in a details page you get redirected to a list page. E.g., I'm in pod details, I change namespace, so I get redirected to pod list page. I'll log an issue about this to make sure this problem is solved. But I dont want to make this PR bigger :)

Issue: #789

---

src/app/frontend/logs/logs_stateconfig.js, line 19 [r3] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Just in the line of my previous comment: does it make sense from a user perspective to change the namespace on the logs page? Given that the rcNamespace and podId are already fixed in the URL and a namespace change in the toolbar won't affect anything.

Yes, it does not make sense. Can we fix this in next PR? To provide configuration switch to disable it.

---

src/app/frontend/replicasetdetail/replicasetdetail_module.js, line 35 [r3] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

O.K., the dependencies on namespaceModule in the other modules seem a bit inconsistent. First I thought that you added a dep to namespaceModule only in the resource list modules (e.g. replicasetlist, podlist, etc.). But now I see this dep in a resource detail module. E.g. there's no dependecy on namespaecModule in the poddetail_module.js.

Is there a reason for this?

`poddetail_module.js` should have namespaceModule in deps. This is because it has namespace state as parent. This should have been caught by tests, as in all other modules. It wasnt caught probably because something is not tested in poddetails.

---

Comments from Reviewable

[taimir]

Review status: 28 of 66 files reviewed at latest revision, 3 unresolved discussions.

---

src/app/frontend/common/namespace/namespaceselect_component.js, line 107 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

The only question is, do we need the NAMESPACE_NOT_SELECTED?

This is needed to have correct label in the UI when "no namespace" is selected. Of course, this could be done in other ways, e.g., we could have compound objects as select models (like, {displayString: foo, namespace: bar}), but both solutions are equivalent.

Agreed.

---

src/app/frontend/common/resource/resourcedetail.js, line 36 [r3] (raw file):

But I dont want to make this PR bigger :)

Ditto.

Great, that makes it clear :)

---

src/app/frontend/replicasetdetail/replicasetdetail_module.js, line 35 [r3] (raw file):

Previously, bryk (Piotr Bryk) wrote…

poddetail_module.js should have namespaceModule in deps. This is because it has namespace state as parent. This should have been caught by tests, as in all other modules. It wasnt caught probably because something is not tested in poddetails.

Ouch ... that about the tests sounds worrisome. Please check the other detail modules as well, I think `replicationcontrollerdetail` is one instance where it's missing.

---

Comments from Reviewable

[taimir]

LGTM so far, I'll review the tests tomorrow morning :)

Previously, bryk (Piotr Bryk) wrote…

PTAL

---

Reviewed 1 of 57 files at r1, 16 of 19 files at r4, 10 of 10 files at r5.
Review status: 55 of 66 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

src/app/frontend/common/namespace/namespace_stateconfig.js, line 20 [r5] (raw file):

/**
 * namespace is an abstract state with no path, but with one parameter ?namespace= that
 * is always be accepted (since namespace is above all).

is always be is wrong grammar-wise (sorry, that's my bad :D)

---

Comments from Reviewable

[taimir]

Oh, I almost forgot: so where is this deploy button after all :D ?

Previously, taimir (Atanas Mirchev) wrote…

LGTM so far, I'll review the tests tomorrow morning :)

---

Review status: 55 of 66 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[bryk]

Addressed comments. PTAL

Previously, taimir (Atanas Mirchev) wrote…

Oh, I almost forgot: so where is this deploy button after all :D ?

---

Review status: 52 of 70 files reviewed at latest revision, 2 unresolved discussions.

---

src/app/frontend/common/namespace/namespace_stateconfig.js, line 20 [r5] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

is always be is wrong grammar-wise (sorry, that's my bad :D)

Yeah.... :)

---

src/app/frontend/replicasetdetail/replicasetdetail_module.js, line 35 [r3] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Ouch ... that about the tests sounds worrisome. Please check the other detail modules as well, I think replicationcontrollerdetail is one instance where it's missing.

Added to all missing modules. Most of them are tested, so we have that enforced. Only a few modules don't have tests for states.

---

Comments from Reviewable

[taimir]

Reviewed 11 of 57 files at r1, 7 of 7 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail_module.js, line 16 [r6] (raw file):

import componentsModule from 'common/components/components_module';
import namespaceModule from 'common/components/namespace_module';

should be common/namespace/namespace_module

---

src/test/frontend/common/namespace/namespace_stateconfig_test.js, line 29 [r6] (raw file):

    angular.mock.module(fakeModule.name);

    let initInjector = () => { angular.mock.inject(($state) => { $state.go('.'); }); };

Maybe

$state.go('fakeState')

right now I am not sure if the '.' actually points to 'fakeState' or to the root.

---

src/test/frontend/common/namespace/namespaceselect_component_test.js, line 97 [r6] (raw file):

  });

  it('should initialize from exisitng namespace and watch for state changes', () => {

Is this a copy of the previous test (maybe a left-over)?

It has the same name, and the only difference is the very last ctrl.loadNamespacesIfNeeded() call that ensures that there's no further get request (and also that you use StateParams('a') instead of namespace: 'a' at one point).

---

src/test/frontend/workloads/workloads_stateconfig_test.js, line 21 [r6] (raw file):

  beforeEach(() => { angular.mock.module(workloadListModule.name); });

  it('should resolve replication controllers with no namespace', angular.mock.inject(($q) => {

Rename to workloads?

---

src/test/frontend/workloads/workloads_stateconfig_test.js, line 33 [r6] (raw file):

  }));

  it('should resolve replication controllers', angular.mock.inject(($q) => {

Rename to workloads?

---

Comments from Reviewable

[taimir]

Last comments I think, PTAL

[bryk]

@taimir Sorry for confusion. I will have yet another version in like an hour. To fix the problem with hiding actionbar. I'll let you know when it is ready.

[bryk]

Last PTAL :)

I've made changes to fix the problem with disappearing action bar. Now there is chromeState instead of namespaceState. It does the same thing, but our template is no longer <div ui-view></div>, but chrome.html. With this we preserve all subview definitions (like actionbar).

Previously, bryk (Piotr Bryk) wrote…

@taimir Sorry for confusion. I will have yet another version in like an hour. To fix the problem with hiding actionbar. I'll let you know when it is ready.

---

Review status: 27 of 72 files reviewed at latest revision, 5 unresolved discussions.

---

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail_module.js, line 16 [r6] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

should be common/namespace/namespace_module

Done.

---

src/test/frontend/common/namespace/namespaceselect_component_test.js, line 97 [r6] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Is this a copy of the previous test (maybe a left-over)?

It has the same name, and the only difference is the very last ctrl.loadNamespacesIfNeeded() call that ensures that there's no further get request (and also that you use StateParams('a') instead of namespace: 'a' at one point).

Correct. Removed :)

---

src/test/frontend/workloads/workloads_stateconfig_test.js, line 21 [r6] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Rename to workloads?

Done.

---

src/test/frontend/workloads/workloads_stateconfig_test.js, line 33 [r6] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Rename to workloads?

Done.

---

src/test/frontend/common/namespace/namespace_stateconfig_test.js, line 29 [r6] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Maybe

$state.go('fakeState')

right now I am not sure if the '.' actually points to 'fakeState' or to the root.

Changed :)

---

Comments from Reviewable

[taimir]

Previously, bryk (Piotr Bryk) wrote…

Last PTAL :)

I've made changes to fix the problem with disappearing action bar. Now there is chromeState instead of namespaceState. It does the same thing, but our template is no longer <div ui-view></div>, but chrome.html. With this we preserve all subview definitions (like actionbar).

---

Reviewed 42 of 48 files at r7, 5 of 5 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

src/app/backend/resource/common/resourcechannels.go, line 300 [r8] (raw file):

          }
      }
      list.Items = filteredItems

Nice catch :) 👍

---

Comments from Reviewable

[taimir]

There's some integration test failures that are flaky maybe (or need to be fixed), other than that great stuff :)

[floreks]

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

src/app/backend/resource/common/resourcechannels.go, line 300 [r8] (raw file):

Previously, taimir (Atanas Mirchev) wrote…

Nice catch :) 👍

Great. I've also fixed that in my PR :)

---

Comments from Reviewable

[bryk]

All right. Thanks for comprehensive review @taimir! I'll fix travis and merge now.

[bryk]

Reviewed 3 of 3 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[bryk]

Reviewed 21 of 59 files at r1, 1 of 20 files at r4, 2 of 11 files at r5, 39 of 48 files at r7, 5 of 5 files at r8, 7 of 7 files at r10.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bryk]

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bryk]

Reviewed 7 of 7 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable