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.

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

ui: persist node drain settings #11368

Merged
merged 3 commits into from
Oct 22, 2021
Merged

ui: persist node drain settings #11368

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b58f27a
ui: persist node drain setting
lgfa29 Oct 21, 2021
984c96c
changelog: add entry for #11368
lgfa29 Oct 21, 2021
0eaa94a
ui: handle zero value drain duration
lgfa29 Oct 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/11368.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
ui: Persist node drain settings in the browser
```
26 changes: 26 additions & 0 deletions ui/app/components/drain-popover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { task } from 'ember-concurrency';
import Duration from 'duration-js';
import { tagName } from '@ember-decorators/component';
import classic from 'ember-classic-decorator';
import localStorageProperty from 'nomad-ui/utils/properties/local-storage';

@classic
@tagName('')
Expand All @@ -22,6 +23,23 @@ export default class DrainPopover extends Component {
forceDrain = false;
drainSystemJobs = true;

@localStorageProperty('nomadDrainOptions', {}) drainOptions;

didReceiveAttrs() {
// Load drain config values from local storage if availabe.
[
'deadlineEnabled',
'customDuration',
'forceDrain',
'drainSystemJobs',
'selectedDurationQuickOption',
].forEach(k => {
if (k in this.drainOptions) {
this[k] = this.drainOptions[k];
}
});
}

@overridable(function() {
return this.durationQuickOptions[0];
})
Expand Down Expand Up @@ -71,6 +89,14 @@ export default class DrainPopover extends Component {
IgnoreSystemJobs: !this.drainSystemJobs,
};

this.drainOptions = {
deadlineEnabled: this.deadlineEnabled,
customDuration: this.deadline,
selectedDurationQuickOption: this.selectedDurationQuickOption,
drainSystemJobs: this.drainSystemJobs,
forceDrain: this.forceDrain,
};

close();

try {
Expand Down
1 change: 1 addition & 0 deletions ui/app/templates/components/drain-popover.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
type="text"
class="input {{if this.parseError "is-danger"}}"
placeholder="1h30m"
value="{{this.customDuration}}"
lgfa29 marked this conversation as resolved.
Show resolved Hide resolved
oninput={{action (queue
(action (mut this.parseError) '')
(action (mut this.customDuration) value="target.value"))}} />
Expand Down
45 changes: 44 additions & 1 deletion ui/tests/acceptance/client-detail-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,18 @@ let clientToken;
const wasPreemptedFilter = allocation => !!allocation.preemptedByAllocation;

function nonSearchPOSTS() {
return server.pretender.handledRequests.reject(request => request.url.includes('fuzzy')).filterBy('method', 'POST');
return server.pretender.handledRequests
.reject(request => request.url.includes('fuzzy'))
.filterBy('method', 'POST');
}

module('Acceptance | client detail', function(hooks) {
setupApplicationTest(hooks);
setupMirage(hooks);

hooks.beforeEach(function() {
window.localStorage.clear();
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 a learning question. Why wouldn't we put this into a clean up hook?

hooks.afterEach(function() {
  window.localStorage.clear();
}

Isn't that more in line with what we're trying to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mostly just two different perspectives. One says "make sure we have a clean environment before running the test", the other says "make sure we clean-up after ourselves before we leave".

The only difference is on the edges of the test cases. with an afterEach, the first test may run with in a dirty environment, and with beforeEach the last test case may leave some side effects behind.

To avoid these problems it's important to keep tests consistent, and all other tests modules that use localStorage clears it with beforeEach.


server.create('node', 'forceIPv4', { schedulingEligibility: 'eligible' });
node = server.db.nodes[0];

Expand Down Expand Up @@ -734,6 +738,45 @@ module('Acceptance | client detail', function(hooks) {
);
});

test('starting a drain persists options to localstorage', async function(assert) {
const nodes = server.createList('node', 2, {
drain: false,
schedulingEligibility: 'eligible',
});

await ClientDetail.visit({ id: nodes[0].id });
await ClientDetail.drainPopover.toggle();

// Change all options to non-default values.
await ClientDetail.drainPopover.deadlineToggle.toggle();
await ClientDetail.drainPopover.deadlineOptions.open();
const optionsCount = ClientDetail.drainPopover.deadlineOptions.options.length;
await ClientDetail.drainPopover.deadlineOptions.options.objectAt(optionsCount - 1).choose();
await ClientDetail.drainPopover.setCustomDeadline('1h40m20s');
await ClientDetail.drainPopover.forceDrainToggle.toggle();
await ClientDetail.drainPopover.systemJobsToggle.toggle();

await ClientDetail.drainPopover.submit();

const got = JSON.parse(window.localStorage.nomadDrainOptions);
const want = {
deadlineEnabled: true,
customDuration: '1h40m20s',
selectedDurationQuickOption: { label: 'Custom', value: 'custom' },
drainSystemJobs: false,
forceDrain: true,
};
assert.deepEqual(got, want);

// Visit another node and check that drain config is persisted.
await ClientDetail.visit({ id: nodes[1].id });
await ClientDetail.drainPopover.toggle();
assert.true(ClientDetail.drainPopover.deadlineToggle.isActive);
assert.equal(ClientDetail.drainPopover.customDeadline, '1h40m20s');
assert.true(ClientDetail.drainPopover.forceDrainToggle.isActive);
assert.false(ClientDetail.drainPopover.systemJobsToggle.isActive);
});

test('the drain popover cancel button closes the popover', async function(assert) {
node = server.create('node', {
drain: false,
Expand Down
7 changes: 5 additions & 2 deletions ui/tests/pages/clients/detail.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
fillable,
text,
isPresent,
value,
visitable,
} from 'ember-cli-page-object';

Expand Down Expand Up @@ -118,15 +119,17 @@ export default create({

deadlineToggle: toggle('[data-test-drain-deadline-toggle]'),
deadlineOptions: {
open: clickable('[data-test-drain-deadline-option-select-parent] .ember-power-select-trigger'),
open: clickable(
'[data-test-drain-deadline-option-select-parent] .ember-power-select-trigger'
),
options: collection('.ember-power-select-option', {
label: text(),
choose: clickable(),
}),
},

setCustomDeadline: fillable('[data-test-drain-custom-deadline]'),
customDeadline: attribute('value', '[data-test-drain-custom-deadline]'),
customDeadline: value('[data-test-drain-custom-deadline]'),
forceDrainToggle: toggle('[data-test-force-drain-toggle]'),
systemJobsToggle: toggle('[data-test-system-jobs-toggle]'),

Expand Down