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

UI: Namespaces redesign #10444

Merged
merged 32 commits into from
Apr 29, 2021
Merged

UI: Namespaces redesign #10444

merged 32 commits into from
Apr 29, 2021

Conversation

DingoEatingFuzz
Copy link
Contributor

This rethinks namespaces as a filter on list pages rather than a global setting. Namespaces are still part of job and CSI volume identifiers, so there was lots of plumbing involved.

The biggest net-new feature here is being able to select All (*) to list all jobs or CSI volumes across namespaces.

Jobs list page with new namespaces facet open to show options, including All

This PR has the following in it (in hindsight it should have been multiple PRs, sorry)

  1. New SingleSelectDropdown component that wraps Ember Power Select and gives it a similar API as MultiSelectDropdown
  2. Refactored multi select dropdown stories as facet stories and includes new ones for the single select dropdown
  3. Removes the namespaces selector from the gutter menu
  4. Conditionally adds a namespaces facet to the jobs, optimize, and csi volumes pages (when there are namespaces other than default)
  5. Conditionally adds a namespaces column to jobs and volume tables
  6. Removes some of the namespaces logic from the system service
  7. Changes some data plumbing for jobs and volumes now that query is being used to add the namespace param rather than it being added invisibly within the data layer based on the global namespace setting.

@github-actions
Copy link

github-actions bot commented Apr 24, 2021

Ember Asset Size action

As of 735f056

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +2.62 kB +420 B
nomad-ui.css +1.89 kB +180 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Apr 24, 2021

Ember Test Audit comparison

main 735f056 change
passes 1078 1078 0
failures 0 0 0
flaky 1 0 -1
duration 7m 55s 868ms 7m 41s 915ms -13s 953ms

Copy link
Contributor

@backspace backspace left a comment

Choose a reason for hiding this comment

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

I left a few comments but nothing truly blocking.

I tried this out locally, though it’s extensive and I’m not sure I exercised everything.

The behaviour with cachedNamespace is a bit surprising maybe, because it’s only used before a controller has been visited; subsequent visits have the same namespace instead of the shared/cached one. But that’s just query parameters for you!

I noticed that pagination might need resetting when the filter has caused the collection to shrink:

pagination-persistence

But then it’s also a thing with other facets so we can declare that out of scope 😆

other-pagination-persistence

{{/let}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Quite minor but is .editorconfig not working somehow 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this about the lack of newline? I know I don't have the editor config vim plugin. I should get it though...

Copy link
Contributor

Choose a reason for hiding this comment

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

ya it’s configured to not add newlines in Handlebars files; it probably doesn’t matter in this case but sometimes it matters


&[aria-selected='true'] {
background: $blue-050;
}
}

.dropdown-empty {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is barely noticeable but I saw that a descender in the namespace dropdown was truncated:

namespace-truncation

It’s more prominent in Storybook:
storybook-truncation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I noticed that but let it slide 😞 I think it's some default ember-power-selector styles that need to be overwritten.

label: text(),
}),
facets: {
namespace: singleFacet('[data-test-namespace-facet]'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice that you were able to adapt the existing interface 🤩

@@ -113,37 +113,6 @@ export default class SystemService extends Service {
return namespaces.length && namespaces.some(namespace => namespace.get('id') !== 'default');
}

@computed('namespaces.[]')
Copy link
Contributor

Choose a reason for hiding this comment

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

Since system.cachedNamespace is set and used elsewhere, what do you think of adding a “declaration” for it, and maybe an explanatory comment?

In the same style as the multi select dropdown
This way it includes all of the facet components (right now multi select
and single select).
Either deleting irrelevant tests or marking failing tests as TODO
pending new namespaces implementation.
… ones

This is the bulk of this refactor: Namespace should no longer be global
because lists should support various namespace configurations.
Furthermore, namespace is an attribute of the ID of a job but a filter
on the jobs list page. Therefore, they should be modeled differently.
This can happen if a job doesn't have a namespace (which shouldn't
actually ever happen, but it can in tests).
@DingoEatingFuzz
Copy link
Contributor Author

I documented the cachedNamespace property, and while doing so a flaky test discovered a bug, so I fixed that too.

@github-actions
Copy link

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on main:

  • Integration | Utility | exec-command-editor-xterm-adapter: it supports typing ^U to delete the entire command

@backspace backspace merged commit 9239168 into main Apr 29, 2021
@backspace backspace deleted the f-ui/namespaces-redesign branch April 29, 2021 20:01
Comment on lines +19 to 23
get _namespace() {
if (!this.namespace) return 'default';
if (typeof this.namespace === 'string') return this.namespace;
return get(this.namespace, 'name');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

activeNamespace was previously set in our models based on query parameters. We're noticing that we can't detect namespace-specific policy capabilities (with ACL's).

Should we continue the behavior of setting system.activeNamespace in our models to grab namespaces from the query params.

Also, since we're setting namespace to default on line 17, what's the purpose of the control flow logic on lines 19-22.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can set this value based on qpNamespace

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants