From ccae1fb1d63c649b7c4743e7c496721acce04e32 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 2 Aug 2018 15:54:57 -0700 Subject: [PATCH 01/32] Bare minimum Mirage support for regions --- ui/mirage/config.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ui/mirage/config.js b/ui/mirage/config.js index 55293e57452..fa7c1f6a1a1 100644 --- a/ui/mirage/config.js +++ b/ui/mirage/config.js @@ -222,6 +222,10 @@ export default function() { return new Response(403, {}, null); }); + this.get('/regions', function() { + return JSON.stringify(['global']); + }); + const clientAllocationStatsHandler = function({ clientAllocationStats }, { params }) { return this.serialize(clientAllocationStats.find(params.id)); }; From dc1a031460333a6508d0dbb7438b2325b6bdbc11 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 2 Aug 2018 15:56:11 -0700 Subject: [PATCH 02/32] Add three-way region property (query param, service, localStorage) --- ui/app/controllers/application.js | 20 +++++++++++++++++ ui/app/routes/application.js | 22 +++++++++++++++++++ ui/app/services/system.js | 36 +++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+) diff --git a/ui/app/controllers/application.js b/ui/app/controllers/application.js index fc3c5e58e92..69d3bcbff25 100644 --- a/ui/app/controllers/application.js +++ b/ui/app/controllers/application.js @@ -7,6 +7,16 @@ import codesForError from '../utils/codes-for-error'; export default Controller.extend({ config: service(), + system: service(), + + queryParams: { + region: 'region', + }, + + region: 'global', + + syncRegionService: forwardRegion('region', 'system.activeRegion'), + syncRegionParam: forwardRegion('system.activeRegion', 'region'), error: null, @@ -43,3 +53,13 @@ export default Controller.extend({ } }), }); + +function forwardRegion(source, destination) { + return observer(source, function() { + const newRegion = this.get(source); + const currentRegion = this.get(destination); + if (currentRegion !== newRegion) { + this.set(destination, newRegion); + } + }); +} diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index 469e76a0af9..c4eea77f0ac 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -1,9 +1,11 @@ import { inject as service } from '@ember/service'; import Route from '@ember/routing/route'; +import { next } from '@ember/runloop'; import { AbortError } from 'ember-data/adapters/errors'; export default Route.extend({ config: service(), + system: service(), resetController(controller, isExiting) { if (isExiting) { @@ -11,6 +13,26 @@ export default Route.extend({ } }, + beforeModel() { + return this.get('system.regions'); + }, + + syncToController(controller) { + const region = this.get('system.activeRegion'); + + // The run next is necessary to let the controller figure + // itself out before updating QPs. + // See: https://github.com/emberjs/ember.js/issues/5465 + next(() => { + controller.set('region', region || 'global'); + }); + }, + + setupController(controller) { + this.syncToController(controller); + return this._super(...arguments); + }, + actions: { didTransition() { if (!this.get('config.isTest')) { diff --git a/ui/app/services/system.js b/ui/app/services/system.js index cb0e84b9a58..93a1744f669 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -1,6 +1,7 @@ import Service, { inject as service } from '@ember/service'; import { computed } from '@ember/object'; import PromiseObject from '../utils/classes/promise-object'; +import PromiseArray from '../utils/classes/promise-array'; import { namespace } from '../adapters/application'; export default Service.extend({ @@ -23,6 +24,41 @@ export default Service.extend({ }); }), + regions: computed(function() { + const token = this.get('token'); + + return PromiseArray.create({ + promise: token.authorizedRequest(`/${namespace}/regions`).then(res => res.json()), + }); + }), + + activeRegion: computed('regions.[]', { + get() { + const regions = this.get('regions'); + const region = window.localStorage.nomadActiveRegion; + + if (regions.includes(region)) { + return region; + } + + // If the region in localStorage is no longer in the cluster, it needs to + // be cleared from localStorage + this.set('activeRegion', null); + return null; + }, + set(key, value) { + if (value == null) { + window.localStorage.removeItem('nomadActiveRegion'); + } else { + // All localStorage values are strings. Stringify first so + // the return value is consistent with what is persisted. + const strValue = value + ''; + window.localStorage.nomadActiveRegion = strValue; + return strValue; + } + }, + }), + namespaces: computed(function() { return this.get('store').findAll('namespace'); }), From 27308a884c362f5e4e74f4cf6f089192fef978cc Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Fri, 3 Aug 2018 10:11:47 -0700 Subject: [PATCH 03/32] Styles for the region switcher --- ui/app/styles/app.scss | 5 +-- ui/app/styles/components.scss | 1 + ui/app/styles/components/dropdown.scss | 35 +++++++++++++++++++ ui/app/styles/core/navbar.scss | 9 +++++ ui/app/templates/components/global-header.hbs | 12 ++++++- 5 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 ui/app/styles/components/dropdown.scss diff --git a/ui/app/styles/app.scss b/ui/app/styles/app.scss index 952709aee32..73ece329ef7 100644 --- a/ui/app/styles/app.scss +++ b/ui/app/styles/app.scss @@ -1,8 +1,9 @@ @import './core'; -@import './components'; -@import './charts'; @import 'ember-power-select'; +@import './components'; +@import './charts'; + // Only necessary in dev @import './styleguide.scss'; diff --git a/ui/app/styles/components.scss b/ui/app/styles/components.scss index be9bf7c74c1..93bbf906e0c 100644 --- a/ui/app/styles/components.scss +++ b/ui/app/styles/components.scss @@ -2,6 +2,7 @@ @import './components/badge'; @import './components/boxed-section'; @import './components/cli-window'; +@import './components/dropdown'; @import './components/ember-power-select'; @import './components/empty-message'; @import './components/error-container'; diff --git a/ui/app/styles/components/dropdown.scss b/ui/app/styles/components/dropdown.scss new file mode 100644 index 00000000000..3b59d6950bf --- /dev/null +++ b/ui/app/styles/components/dropdown.scss @@ -0,0 +1,35 @@ +.ember-power-select-trigger { + padding: 0.3em 16px 0.3em 0.3em; + border-radius: $radius; + box-shadow: $button-box-shadow-standard; + + &.is-outlined { + border-color: rgba($white, 0.5); + color: $white; + background: transparent; + box-shadow: $button-box-shadow-standard, 0 0 2px 2px rgba($black, 0.1); + + .ember-power-select-status-icon { + border-top-color: rgba($white, 0.75); + } + + .ember-power-select-prefix { + color: rgba($white, 0.75); + } + } +} + +.ember-power-select-selected-item { + text-overflow: ellipsis; + white-space: nowrap; +} + +.ember-power-select-prefix { + color: $grey; +} + +.ember-power-select-option { + .ember-power-select-prefix { + display: none; + } +} diff --git a/ui/app/styles/core/navbar.scss b/ui/app/styles/core/navbar.scss index 4ab73cfe8b5..6d1926cde21 100644 --- a/ui/app/styles/core/navbar.scss +++ b/ui/app/styles/core/navbar.scss @@ -75,6 +75,15 @@ &.is-gutter { width: $gutter-width; + display: block; + padding: 0 1rem; + font-size: 1em; + + // Unfortunate necessity to middle align an element larger than + // plain text in the subnav. + > * { + margin: -5px 0; + } @media #{$mq-hidden-gutter} { width: 0; diff --git a/ui/app/templates/components/global-header.hbs b/ui/app/templates/components/global-header.hbs index 50cb7752eef..c371031213d 100644 --- a/ui/app/templates/components/global-header.hbs +++ b/ui/app/templates/components/global-header.hbs @@ -16,7 +16,17 @@
-
+
+ {{#power-select + data-test-region-switcher + triggerClass="is-outlined" + options=sortedRegions + selected=system.activeRegion + searchEnabled=false + onchange=(action gotoRegion) as |region|}} + Region: {{region}} + {{/power-select}} +
- {{#power-select - data-test-region-switcher - triggerClass="is-outlined" - options=sortedRegions - selected=system.activeRegion - searchEnabled=false - onchange=(action gotoRegion) as |region|}} - Region: {{region}} - {{/power-select}} + {{#if system.shouldShowRegions}} + {{#power-select + data-test-region-switcher + triggerClass="is-outlined" + options=sortedRegions + selected=system.activeRegion + searchEnabled=false + onchange=(action gotoRegion) as |region|}} + Region: {{region}} + {{/power-select}} + {{/if}}
- {{#if system.shouldShowRegions}} - {{#power-select - data-test-region-switcher - triggerClass="is-outlined" - options=sortedRegions - selected=system.activeRegion - searchEnabled=false - onchange=(action gotoRegion) as |region|}} - Region: {{region}} - {{/power-select}} - {{/if}} + {{region-switcher decoration="is-outlined"}}
+
{{else}}
-

No Allocations

-

No allocations have been placed.

+

No Matches

+

No allocations match the term {{searchTerm}}

- {{/if}} - -{{/gutter-menu}} - + {{/list-pagination}} + {{else}} +
+
+

No Allocations

+

No allocations have been placed.

+
+
+ {{/if}} + From 7e6f02d0c00a38bdd753c1736d79e1ee8f8d668a Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 6 Aug 2018 18:33:00 -0700 Subject: [PATCH 15/32] Account for the service:system dependency due to region --- ui/tests/unit/adapters/node-test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/tests/unit/adapters/node-test.js b/ui/tests/unit/adapters/node-test.js index 048f8d3bd86..67e0e8c30d3 100644 --- a/ui/tests/unit/adapters/node-test.js +++ b/ui/tests/unit/adapters/node-test.js @@ -15,6 +15,7 @@ moduleForAdapter('node', 'Unit | Adapter | Node', { 'model:job', 'serializer:application', 'serializer:node', + 'service:system', 'service:token', 'service:config', 'service:watchList', From 4715696959ce2ce7575f0ec98b8d9769cf877937 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 6 Aug 2018 18:33:57 -0700 Subject: [PATCH 16/32] Clear up the data flow for namespaces This is incredibly tricky with query params, since there is a bundle of timing issues, lifecycle issues, missing features, and all around gotchas with query params. This solution has no observers and no instances of the system service being set from the jobs controller. The upside to this is no observers, much easier to follow logic, no more dependent key chain reactions. --- ui/app/controllers/jobs.js | 24 -------- ui/app/routes/jobs.js | 52 ++++++++++------ ui/app/services/system.js | 2 +- ui/tests/acceptance/task-detail-test.js | 8 ++- ui/tests/unit/adapters/job-test.js | 80 ++++++++++++++----------- 5 files changed, 86 insertions(+), 80 deletions(-) diff --git a/ui/app/controllers/jobs.js b/ui/app/controllers/jobs.js index c8fd48cf5ef..b38d2b11b12 100644 --- a/ui/app/controllers/jobs.js +++ b/ui/app/controllers/jobs.js @@ -1,7 +1,5 @@ import { inject as service } from '@ember/service'; import Controller from '@ember/controller'; -import { observer } from '@ember/object'; -import { run } from '@ember/runloop'; export default Controller.extend({ system: service(), @@ -13,26 +11,4 @@ export default Controller.extend({ isForbidden: false, jobNamespace: 'default', - - // The namespace query param should act as an alias to the system active namespace. - // But query param defaults can't be CPs: https://github.com/emberjs/ember.js/issues/9819 - syncNamespaceService: forwardNamespace('jobNamespace', 'system.activeNamespace'), - syncNamespaceParam: forwardNamespace('system.activeNamespace', 'jobNamespace'), }); - -function forwardNamespace(source, destination) { - return observer(source, `${source}.id`, function() { - const newNamespace = this.get(`${source}.id`) || this.get(source); - const currentNamespace = this.get(`${destination}.id`) || this.get(destination); - const bothAreDefault = - (currentNamespace == undefined || currentNamespace === 'default') && - (newNamespace == undefined || newNamespace === 'default'); - - if (currentNamespace !== newNamespace && !bothAreDefault) { - this.set(destination, newNamespace); - run.next(() => { - this.send('refreshRoute'); - }); - } - }); -} diff --git a/ui/app/routes/jobs.js b/ui/app/routes/jobs.js index 20cca8d58e1..beeb0825660 100644 --- a/ui/app/routes/jobs.js +++ b/ui/app/routes/jobs.js @@ -1,6 +1,6 @@ import { inject as service } from '@ember/service'; +import { next } from '@ember/runloop'; import Route from '@ember/routing/route'; -import { run } from '@ember/runloop'; import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state'; import notifyForbidden from 'nomad-ui/utils/notify-forbidden'; @@ -15,32 +15,50 @@ export default Route.extend(WithForbiddenState, { }, ], - model() { - return this.get('store') - .findAll('job', { reload: true }) - .catch(notifyForbidden(this)); + queryParams: { + jobNamespace: { + refreshModel: true, + }, + }, + + afterSetup(fn) { + this._afterSetups || (this._afterSetups = []); + this._afterSetups.push(fn); }, - syncToController(controller) { - const namespace = this.get('system.activeNamespace.id'); - - // The run next is necessary to let the controller figure - // itself out before updating QPs. - // See: https://github.com/emberjs/ember.js/issues/5465 - run.next(() => { - if (namespace && namespace !== 'default') { - controller.set('jobNamespace', namespace); - } else { - controller.set('jobNamespace', 'default'); + beforeModel(transition) { + return this.get('system.namespaces').then(namespaces => { + const queryParam = transition.queryParams.namespace; + const activeNamespace = this.get('system.activeNamespace.id'); + + if (!queryParam && activeNamespace !== 'default') { + this.afterSetup(controller => { + controller.set('jobNamespace', activeNamespace); + }); + } else if (queryParam && queryParam !== activeNamespace) { + this.set('system.activeNamespace', queryParam); } + + return namespaces; }); }, setupController(controller) { - this.syncToController(controller); + next(() => { + (this._afterSetups || []).forEach(fn => { + fn(controller); + }); + this._afterSetups = []; + }); return this._super(...arguments); }, + model() { + return this.get('store') + .findAll('job', { reload: true }) + .catch(notifyForbidden(this)); + }, + actions: { refreshRoute() { this.refresh(); diff --git a/ui/app/services/system.js b/ui/app/services/system.js index 5e0bc5b71e2..9f7f74c5eb8 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -43,7 +43,7 @@ export default Service.extend({ // If the region in localStorage is no longer in the cluster, it needs to // be cleared from localStorage - this.set('activeRegion', null); + // this.set('activeRegion', null); return null; }, set(key, value) { diff --git a/ui/tests/acceptance/task-detail-test.js b/ui/tests/acceptance/task-detail-test.js index c4e43c56c6d..cc30097f708 100644 --- a/ui/tests/acceptance/task-detail-test.js +++ b/ui/tests/acceptance/task-detail-test.js @@ -166,9 +166,11 @@ test('when the allocation is found but the task is not, the application errors', Task.visit({ id: allocation.id, name: 'not-a-real-task-name' }); andThen(() => { - assert.equal( - server.pretender.handledRequests.findBy('status', 200).url, - `/v1/allocation/${allocation.id}`, + assert.ok( + server.pretender.handledRequests + .filterBy('status', 200) + .mapBy('url') + .includes(`/v1/allocation/${allocation.id}`), 'A request to the allocation is made successfully' ); assert.equal( diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index 1eca17e933a..9b96b45d187 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -58,13 +58,15 @@ test('The job endpoint is the only required endpoint for fetching a job', functi const jobNamespace = 'default'; const jobId = JSON.stringify([jobName, jobNamespace]); - this.subject().findRecord(null, { modelName: 'job' }, jobId); + return wait().then(() => { + this.subject().findRecord(null, { modelName: 'job' }, jobId); - assert.deepEqual( - pretender.handledRequests.mapBy('url'), - [`/v1/job/${jobName}`], - 'The only request made is /job/:id' - ); + assert.deepEqual( + pretender.handledRequests.mapBy('url'), + [`/v1/job/${jobName}`], + 'The only request made is /job/:id' + ); + }); }); test('When a namespace is set in localStorage but a job in the default namespace is requested, the namespace query param is not present', function(assert) { @@ -81,8 +83,8 @@ test('When a namespace is set in localStorage but a job in the default namespace assert.deepEqual( pretender.handledRequests.mapBy('url'), - [`/v1/job/${jobName}`], - 'The one request made is /job/:id with no namespace query param' + ['/v1/namespaces', `/v1/job/${jobName}`], + 'The only requests made are /namespaces and /job/:id with no namespace query param' ); }); }); @@ -95,13 +97,15 @@ test('When a namespace is in localStorage and the requested job is in the defaul const jobNamespace = 'default'; const jobId = JSON.stringify([jobName, jobNamespace]); - this.subject().findRecord(null, { modelName: 'job' }, jobId); + return wait().then(() => { + this.subject().findRecord(null, { modelName: 'job' }, jobId); - assert.deepEqual( - pretender.handledRequests.mapBy('url'), - [`/v1/job/${jobName}`], - 'The request made is /job/:id with no namespace query param' - ); + assert.deepEqual( + pretender.handledRequests.mapBy('url'), + [`/v1/job/${jobName}`], + 'The request made is /job/:id with no namespace query param' + ); + }); }); test('When the job has a namespace other than default, it is in the URL', function(assert) { @@ -110,25 +114,29 @@ test('When the job has a namespace other than default, it is in the URL', functi const jobNamespace = 'some-namespace'; const jobId = JSON.stringify([jobName, jobNamespace]); - this.subject().findRecord(null, { modelName: 'job' }, jobId); + return wait().then(() => { + this.subject().findRecord(null, { modelName: 'job' }, jobId); - assert.deepEqual( - pretender.handledRequests.mapBy('url'), - [`/v1/job/${jobName}?namespace=${jobNamespace}`], - 'The only request made is /job/:id?namespace=:namespace' - ); + assert.deepEqual( + pretender.handledRequests.mapBy('url'), + [`/v1/job/${jobName}?namespace=${jobNamespace}`], + 'The only request made is /job/:id?namespace=:namespace' + ); + }); }); test('When there is no token set in the token service, no x-nomad-token header is set', function(assert) { const { pretender } = this.server; const jobId = JSON.stringify(['job-1', 'default']); - this.subject().findRecord(null, { modelName: 'job' }, jobId); + return wait().then(() => { + this.subject().findRecord(null, { modelName: 'job' }, jobId); - assert.notOk( - pretender.handledRequests.mapBy('requestHeaders').some(headers => headers['X-Nomad-Token']), - 'No token header present on either job request' - ); + assert.notOk( + pretender.handledRequests.mapBy('requestHeaders').some(headers => headers['X-Nomad-Token']), + 'No token header present on either job request' + ); + }); }); test('When a token is set in the token service, then x-nomad-token header is set', function(assert) { @@ -136,15 +144,17 @@ test('When a token is set in the token service, then x-nomad-token header is set const jobId = JSON.stringify(['job-1', 'default']); const secret = 'here is the secret'; - this.subject().set('token.secret', secret); - this.subject().findRecord(null, { modelName: 'job' }, jobId); + return wait().then(() => { + this.subject().set('token.secret', secret); + this.subject().findRecord(null, { modelName: 'job' }, jobId); - assert.ok( - pretender.handledRequests - .mapBy('requestHeaders') - .every(headers => headers['X-Nomad-Token'] === secret), - 'The token header is present on both job requests' - ); + assert.ok( + pretender.handledRequests + .mapBy('requestHeaders') + .every(headers => headers['X-Nomad-Token'] === secret), + 'The token header is present on both job requests' + ); + }); }); test('findAll can be watched', function(assert) { @@ -158,7 +168,7 @@ test('findAll can be watched', function(assert) { request(); assert.equal( - pretender.handledRequests[0].url, + pretender.handledRequests[1].url, '/v1/jobs?index=1', 'Second request is a blocking request for jobs' ); @@ -166,7 +176,7 @@ test('findAll can be watched', function(assert) { return wait().then(() => { request(); assert.equal( - pretender.handledRequests[1].url, + pretender.handledRequests[2].url, '/v1/jobs?index=2', 'Third request is a blocking request with an incremented index param' ); From a5da73da31644019ef936e6155680d293a0ba420 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 8 Aug 2018 13:49:04 -0700 Subject: [PATCH 17/32] Repeat the new namespace pattern for region --- ui/app/controllers/application.js | 15 +------------- ui/app/routes/application.js | 34 +++++++++++++++++++++---------- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/ui/app/controllers/application.js b/ui/app/controllers/application.js index 69d3bcbff25..15902ebd80b 100644 --- a/ui/app/controllers/application.js +++ b/ui/app/controllers/application.js @@ -13,10 +13,7 @@ export default Controller.extend({ region: 'region', }, - region: 'global', - - syncRegionService: forwardRegion('region', 'system.activeRegion'), - syncRegionParam: forwardRegion('system.activeRegion', 'region'), + region: null, error: null, @@ -53,13 +50,3 @@ export default Controller.extend({ } }), }); - -function forwardRegion(source, destination) { - return observer(source, function() { - const newRegion = this.get(source); - const currentRegion = this.get(destination); - if (currentRegion !== newRegion) { - this.set(destination, newRegion); - } - }); -} diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index 93bb577dd90..02893d17ce1 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -1,6 +1,6 @@ import { inject as service } from '@ember/service'; -import Route from '@ember/routing/route'; import { next } from '@ember/runloop'; +import Route from '@ember/routing/route'; import { AbortError } from 'ember-data/adapters/errors'; import RSVP from 'rsvp'; @@ -20,23 +20,35 @@ export default Route.extend({ } }, - beforeModel() { - return RSVP.all([this.get('system.regions'), this.get('system.namespaces')]); + afterSetup(fn) { + this._afterSetups || (this._afterSetups = []); + this._afterSetups.push(fn); }, - syncToController(controller) { - const region = this.get('system.activeRegion'); + beforeModel(transition) { + return RSVP.all([this.get('system.regions'), this.get('system.namespaces')]).then(promises => { + const queryParam = transition.queryParams.region; + const activeRegion = this.get('system.activeRegion'); - // The run next is necessary to let the controller figure - // itself out before updating QPs. - // See: https://github.com/emberjs/ember.js/issues/5465 - next(() => { - controller.set('region', region || 'global'); + if (!queryParam && activeRegion) { + this.afterSetup(controller => { + controller.set('region', activeRegion); + }); + } else if (queryParam && queryParam !== activeRegion) { + this.set('system.activeRegion', queryParam); + } + + return promises; }); }, setupController(controller) { - this.syncToController(controller); + next(() => { + (this._afterSetups || []).forEach(fn => { + fn(controller); + }); + this._afterSetups = []; + }); return this._super(...arguments); }, From 61bdc3bfe7243c291d9cf7ed81d6d3456fc8990f Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 8 Aug 2018 16:08:27 -0700 Subject: [PATCH 18/32] Sidestep a transpilation bug. Default params don't make it into arguments which were being splatted into the call to super. --- ui/app/adapters/application.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/app/adapters/application.js b/ui/app/adapters/application.js index 5ab9bef0aa0..c6acb666d4c 100644 --- a/ui/app/adapters/application.js +++ b/ui/app/adapters/application.js @@ -42,7 +42,7 @@ export default RESTAdapter.extend({ if (region) { options.data.region = region; } - return this._super(...arguments); + return this._super(url, type, options); }, // In order to remove stale records from the store, findHasMany has to unload From 4473bb90c5218ea64bc5e59984eb0ac04af40405 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 8 Aug 2018 16:09:06 -0700 Subject: [PATCH 19/32] The application route doesn't need to fetch namespaces --- ui/app/routes/application.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index 02893d17ce1..2506f2dc20a 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -2,7 +2,6 @@ import { inject as service } from '@ember/service'; import { next } from '@ember/runloop'; import Route from '@ember/routing/route'; import { AbortError } from 'ember-data/adapters/errors'; -import RSVP from 'rsvp'; export default Route.extend({ config: service(), @@ -26,7 +25,7 @@ export default Route.extend({ }, beforeModel(transition) { - return RSVP.all([this.get('system.regions'), this.get('system.namespaces')]).then(promises => { + return this.get('system.regions').then(promises => { const queryParam = transition.queryParams.region; const activeRegion = this.get('system.activeRegion'); From c5439df03a6cb771455ecdb9c7583c95a821ad7f Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 8 Aug 2018 16:10:22 -0700 Subject: [PATCH 20/32] Add the region qp to all requests made through the token service --- ui/app/services/system.js | 2 +- ui/app/services/token.js | 28 ++++++++++++++++++++++++++-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/ui/app/services/system.js b/ui/app/services/system.js index 9f7f74c5eb8..8b6be9d9ba4 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -28,7 +28,7 @@ export default Service.extend({ const token = this.get('token'); return PromiseArray.create({ - promise: token.authorizedRequest(`/${namespace}/regions`).then(res => res.json()), + promise: token.authorizedRawRequest(`/${namespace}/regions`).then(res => res.json()), }); }), diff --git a/ui/app/services/token.js b/ui/app/services/token.js index 4ce27d9c354..b4e536e2fc3 100644 --- a/ui/app/services/token.js +++ b/ui/app/services/token.js @@ -1,9 +1,12 @@ -import Service from '@ember/service'; +import Service, { inject as service } from '@ember/service'; import { computed } from '@ember/object'; import { assign } from '@ember/polyfills'; +import queryString from 'query-string'; import fetch from 'nomad-ui/utils/fetch'; export default Service.extend({ + system: service(), + secret: computed({ get() { return window.sessionStorage.nomadTokenSecret; @@ -19,7 +22,12 @@ export default Service.extend({ }, }), - authorizedRequest(url, options = { credentials: 'include' }) { + // All non Ember Data requests should go through authorizedRequest. + // However, the request that gets regions falls into that category. + // This authorizedRawRequest is necessary in order to fetch data + // with the guarantee of a token but without the automatic region + // param since the region cannot be known at this point. + authorizedRawRequest(url, options = { credentials: 'include' }) { const headers = {}; const token = this.get('secret'); @@ -29,4 +37,20 @@ export default Service.extend({ return fetch(url, assign(options, { headers })); }, + + authorizedRequest(url, options) { + const region = this.get('system.activeRegion'); + + if (region) { + url = addParams(url, { region }); + } + + return this.authorizedRawRequest(url, options); + }, }); + +function addParams(url, params) { + const paramsStr = queryString.stringify(params); + const delimiter = url.includes('?') ? '&' : '?'; + return `${url}${delimiter}${paramsStr}`; +} From d690709462f3e0633e920376a30a83b4a1b6cfe2 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 8 Aug 2018 19:34:56 -0700 Subject: [PATCH 21/32] Get the server's region (aka default region) from the API The default region shouldn't be used as a qp since no qp means the same thing. --- ui/app/routes/application.js | 42 +++++++++++++++++++++++++----------- ui/app/routes/jobs.js | 3 ++- ui/app/services/system.js | 23 ++++++++++++++++++++ 3 files changed, 54 insertions(+), 14 deletions(-) diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index 2506f2dc20a..8aed227270b 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -2,6 +2,7 @@ import { inject as service } from '@ember/service'; import { next } from '@ember/runloop'; import Route from '@ember/routing/route'; import { AbortError } from 'ember-data/adapters/errors'; +import RSVP from 'rsvp'; export default Route.extend({ config: service(), @@ -25,23 +26,38 @@ export default Route.extend({ }, beforeModel(transition) { - return this.get('system.regions').then(promises => { - const queryParam = transition.queryParams.region; - const activeRegion = this.get('system.activeRegion'); + return RSVP.all([this.get('system.regions'), this.get('system.defaultRegion')]).then( + promises => { + const queryParam = transition.queryParams.region; + const activeRegion = this.get('system.activeRegion'); + const defaultRegion = this.get('system.defaultRegion.region'); - if (!queryParam && activeRegion) { - this.afterSetup(controller => { - controller.set('region', activeRegion); - }); - } else if (queryParam && queryParam !== activeRegion) { - this.set('system.activeRegion', queryParam); - } + if (!queryParam && activeRegion !== defaultRegion) { + // No query param: use what is in local storage, fallback to defaultRegion + this.afterSetup(controller => { + controller.set('region', activeRegion); + }); + } else if (queryParam && queryParam !== activeRegion) { + // Query param: use the query param, set that value in local storage + this.set('system.activeRegion', queryParam); + if (queryParam === defaultRegion) { + // Query param === default: don't use the query param, set that value in local storage, + // and clear the controller value. + this.afterSetup(controller => { + controller.set('region', null); + }); + } + } - return promises; - }); + return promises; + } + ); }, - setupController(controller) { + // setupController doesn't refire when the model hook refires as part of + // a query param change + afterModel() { + const controller = this.controllerFor('application'); next(() => { (this._afterSetups || []).forEach(fn => { fn(controller); diff --git a/ui/app/routes/jobs.js b/ui/app/routes/jobs.js index beeb0825660..a221735b6be 100644 --- a/ui/app/routes/jobs.js +++ b/ui/app/routes/jobs.js @@ -43,7 +43,8 @@ export default Route.extend(WithForbiddenState, { }); }, - setupController(controller) { + afterModel() { + const controller = this.controllerFor('jobs'); next(() => { (this._afterSetups || []).forEach(fn => { fn(controller); diff --git a/ui/app/services/system.js b/ui/app/services/system.js index 8b6be9d9ba4..5ebb14a1dc6 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -24,6 +24,18 @@ export default Service.extend({ }); }), + defaultRegion: computed(function() { + const token = this.get('token'); + return PromiseObject.create({ + promise: token + .authorizedRawRequest(`/${namespace}/agent/members`) + .then(res => res.json()) + .then(json => { + return { region: json.ServerRegion }; + }), + }); + }), + regions: computed(function() { const token = this.get('token'); @@ -63,6 +75,17 @@ export default Service.extend({ return this.get('regions.length') > 1; }), + shouldIncludeRegion: computed( + 'activeRegion', + 'defaultRegion.region', + 'shouldShowRegions', + function() { + return ( + this.get('shouldShowRegions') && this.get('activeRegion') !== this.get('defaultRegion') + ); + } + ), + namespaces: computed('activeRegion', function() { return PromiseArray.create({ promise: this.get('store') From ae0bf90b7916d5855a3ed017a643c3d98cb3317c Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 8 Aug 2018 19:39:32 -0700 Subject: [PATCH 22/32] Only deal with the region param (in the app and in api calls) when necessary --- ui/app/adapters/application.js | 8 +++++--- ui/app/routes/application.js | 2 ++ ui/app/services/system.js | 3 ++- ui/app/services/token.js | 9 +++++---- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/ui/app/adapters/application.js b/ui/app/adapters/application.js index c6acb666d4c..b1504ff035d 100644 --- a/ui/app/adapters/application.js +++ b/ui/app/adapters/application.js @@ -38,9 +38,11 @@ export default RESTAdapter.extend({ ajaxOptions(url, type, options = {}) { options.data || (options.data = {}); - const region = this.get('system.activeRegion'); - if (region) { - options.data.region = region; + if (this.get('system.shouldIncludeRegion')) { + const region = this.get('system.activeRegion'); + if (region) { + options.data.region = region; + } } return this._super(url, type, options); }, diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index 8aed227270b..a6afcc3868e 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -28,6 +28,8 @@ export default Route.extend({ beforeModel(transition) { return RSVP.all([this.get('system.regions'), this.get('system.defaultRegion')]).then( promises => { + if (!this.get('system.shouldShowRegions')) return promises; + const queryParam = transition.queryParams.region; const activeRegion = this.get('system.activeRegion'); const defaultRegion = this.get('system.defaultRegion.region'); diff --git a/ui/app/services/system.js b/ui/app/services/system.js index 5ebb14a1dc6..6afc8b8b018 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -81,7 +81,8 @@ export default Service.extend({ 'shouldShowRegions', function() { return ( - this.get('shouldShowRegions') && this.get('activeRegion') !== this.get('defaultRegion') + this.get('shouldShowRegions') && + this.get('activeRegion') !== this.get('defaultRegion.region') ); } ), diff --git a/ui/app/services/token.js b/ui/app/services/token.js index b4e536e2fc3..28083a0500c 100644 --- a/ui/app/services/token.js +++ b/ui/app/services/token.js @@ -39,10 +39,11 @@ export default Service.extend({ }, authorizedRequest(url, options) { - const region = this.get('system.activeRegion'); - - if (region) { - url = addParams(url, { region }); + if (this.get('system.shouldIncludeRegion')) { + const region = this.get('system.activeRegion'); + if (region) { + url = addParams(url, { region }); + } } return this.authorizedRawRequest(url, options); From fe315fe6d1f3a22ee529e0fab2710502f2f23c06 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 9 Aug 2018 11:02:50 -0700 Subject: [PATCH 23/32] Address an issue with certain dependent keys --- ui/app/controllers/jobs/index.js | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/ui/app/controllers/jobs/index.js b/ui/app/controllers/jobs/index.js index 0dd0eb5a812..e2bdb4a0106 100644 --- a/ui/app/controllers/jobs/index.js +++ b/ui/app/controllers/jobs/index.js @@ -32,21 +32,17 @@ export default Controller.extend(Sortable, Searchable, { Filtered jobs are those that match the selected namespace and aren't children of periodic or parameterized jobs. */ - filteredJobs: computed( - 'model.[]', - 'model.@each.parent', - 'system.activeNamespace', - 'system.namespaces.length', - function() { - const hasNamespaces = this.get('system.namespaces.length'); - const activeNamespace = this.get('system.activeNamespace.id') || 'default'; - - return this.get('model') - .compact() - .filter(job => !hasNamespaces || job.get('namespace.id') === activeNamespace) - .filter(job => !job.get('parent.content')); - } - ), + filteredJobs: computed('model.[]', 'model.@each.parent', function() { + // Namespace related properties are ommitted from the dependent keys + // due to a prop invalidation bug caused by region switching. + const hasNamespaces = this.get('system.namespaces.length'); + const activeNamespace = this.get('system.activeNamespace.id') || 'default'; + + return this.get('model') + .compact() + .filter(job => !hasNamespaces || job.get('namespace.id') === activeNamespace) + .filter(job => !job.get('parent.content')); + }), listToSort: alias('filteredJobs'), listToSearch: alias('listSorted'), From 822d99833b857c86fa46aaf74d50e7316c7e0e06 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 9 Aug 2018 11:03:37 -0700 Subject: [PATCH 24/32] Handle errors when getting regions or the default regions This protects against volatility in the server and, more commonly, ACLs returning forbidden responses. --- ui/app/services/system.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ui/app/services/system.js b/ui/app/services/system.js index 6afc8b8b018..da7aa10cad5 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -1,9 +1,14 @@ import Service, { inject as service } from '@ember/service'; import { computed } from '@ember/object'; +import { copy } from '@ember/object/internals'; import PromiseObject from '../utils/classes/promise-object'; import PromiseArray from '../utils/classes/promise-array'; import { namespace } from '../adapters/application'; +// When the request isn't ok (e.g., forbidden) handle gracefully +const jsonWithDefault = defaultResponse => res => + res.ok ? res.json() : copy(defaultResponse, true); + export default Service.extend({ token: service(), store: service(), @@ -29,7 +34,7 @@ export default Service.extend({ return PromiseObject.create({ promise: token .authorizedRawRequest(`/${namespace}/agent/members`) - .then(res => res.json()) + .then(jsonWithDefault({})) .then(json => { return { region: json.ServerRegion }; }), @@ -40,7 +45,7 @@ export default Service.extend({ const token = this.get('token'); return PromiseArray.create({ - promise: token.authorizedRawRequest(`/${namespace}/regions`).then(res => res.json()), + promise: token.authorizedRawRequest(`/${namespace}/regions`).then(jsonWithDefault([])), }); }), From b767a8742291f464311f400e306e619f0e1d514b Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 9 Aug 2018 15:35:25 -0700 Subject: [PATCH 25/32] Update tests to handle region switching --- ui/tests/integration/task-log-test.js | 1 + ui/tests/unit/adapters/job-test.js | 14 ++++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/ui/tests/integration/task-log-test.js b/ui/tests/integration/task-log-test.js index 5b2ddab2502..563b0d6323c 100644 --- a/ui/tests/integration/task-log-test.js +++ b/ui/tests/integration/task-log-test.js @@ -56,6 +56,7 @@ moduleForComponent('task-log', 'Integration | Component | task log', { this.server = new Pretender(function() { this.get(`http://${HOST}/v1/client/fs/logs/:allocation_id`, handler); this.get('/v1/client/fs/logs/:allocation_id', handler); + this.get('/v1/regions', () => [200, {}, '[]']); }); }, afterEach() { diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index 9b96b45d187..9b091c33e1a 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -41,7 +41,13 @@ moduleForAdapter('job', 'Unit | Adapter | Job', { this.server.create('job', { id: 'job-2', namespaceId: 'some-namespace' }); this.system = getOwner(this).lookup('service:system'); + + // Namespace, default region, and all regions are requests that all + // job requests depend on. Fetching them ahead of time means testing + // job adapter behavior in isolation. this.system.get('namespaces'); + this.system.get('shouldIncludeRegion'); + this.system.get('defaultRegion'); // Reset the handledRequests array to avoid accounting for this // namespaces request everywhere. @@ -83,8 +89,8 @@ test('When a namespace is set in localStorage but a job in the default namespace assert.deepEqual( pretender.handledRequests.mapBy('url'), - ['/v1/namespaces', `/v1/job/${jobName}`], - 'The only requests made are /namespaces and /job/:id with no namespace query param' + [`/v1/job/${jobName}`], + 'The only request made is /job/:id with no namespace query param' ); }); }); @@ -168,7 +174,7 @@ test('findAll can be watched', function(assert) { request(); assert.equal( - pretender.handledRequests[1].url, + pretender.handledRequests[0].url, '/v1/jobs?index=1', 'Second request is a blocking request for jobs' ); @@ -176,7 +182,7 @@ test('findAll can be watched', function(assert) { return wait().then(() => { request(); assert.equal( - pretender.handledRequests[2].url, + pretender.handledRequests[1].url, '/v1/jobs?index=2', 'Third request is a blocking request with an incremented index param' ); From 362aff0fab943c3edf6d7df8dba53919fa0096c6 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 9 Aug 2018 18:12:26 -0700 Subject: [PATCH 26/32] Simplify the control flow around changing namespaces and regions The UI will no longer try to redirect to the appropriate namespace or region if one is found in localStorage. Instead, it will assume that the lack of query param means the default namespaces or region is desired. --- ui/app/components/region-switcher.js | 9 +--- ui/app/routes/application.js | 48 ++++++++----------- ui/app/routes/jobs.js | 27 +---------- ui/app/services/system.js | 5 +- .../templates/components/region-switcher.hbs | 1 + 5 files changed, 26 insertions(+), 64 deletions(-) diff --git a/ui/app/components/region-switcher.js b/ui/app/components/region-switcher.js index af42ed9a900..41574a908af 100644 --- a/ui/app/components/region-switcher.js +++ b/ui/app/components/region-switcher.js @@ -1,5 +1,4 @@ import Component from '@ember/component'; -import { next } from '@ember/runloop'; import { computed } from '@ember/object'; import { inject as service } from '@ember/service'; @@ -15,12 +14,8 @@ export default Component.extend({ }), gotoRegion(region) { - this.get('system').reset(); - this.get('store').unloadAll(); - next(() => { - this.get('router').transitionTo('jobs', { - queryParams: { region }, - }); + this.get('router').transitionTo('jobs', { + queryParams: { region }, }); }, }); diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index a6afcc3868e..6119eb4cea4 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -7,6 +7,7 @@ import RSVP from 'rsvp'; export default Route.extend({ config: service(), system: service(), + store: service(), queryParams: { region: { @@ -20,37 +21,26 @@ export default Route.extend({ } }, - afterSetup(fn) { - this._afterSetups || (this._afterSetups = []); - this._afterSetups.push(fn); - }, - beforeModel(transition) { return RSVP.all([this.get('system.regions'), this.get('system.defaultRegion')]).then( promises => { if (!this.get('system.shouldShowRegions')) return promises; const queryParam = transition.queryParams.region; - const activeRegion = this.get('system.activeRegion'); const defaultRegion = this.get('system.defaultRegion.region'); + const currentRegion = this.get('system.activeRegion') || defaultRegion; - if (!queryParam && activeRegion !== defaultRegion) { - // No query param: use what is in local storage, fallback to defaultRegion - this.afterSetup(controller => { - controller.set('region', activeRegion); - }); - } else if (queryParam && queryParam !== activeRegion) { - // Query param: use the query param, set that value in local storage - this.set('system.activeRegion', queryParam); - if (queryParam === defaultRegion) { - // Query param === default: don't use the query param, set that value in local storage, - // and clear the controller value. - this.afterSetup(controller => { - controller.set('region', null); - }); - } + // Only reset the store if the region actually changed + if ( + (queryParam && queryParam !== currentRegion) || + (!queryParam && currentRegion !== defaultRegion) + ) { + this.get('system').reset(); + this.get('store').unloadAll(); } + this.set('system.activeRegion', queryParam || defaultRegion); + return promises; } ); @@ -58,14 +48,18 @@ export default Route.extend({ // setupController doesn't refire when the model hook refires as part of // a query param change - afterModel() { + afterModel(model, transition) { + const queryParam = transition.queryParams.region; const controller = this.controllerFor('application'); - next(() => { - (this._afterSetups || []).forEach(fn => { - fn(controller); + + // The default region shouldn't show up as a query param since + // it's superfluous. + if (queryParam === this.get('system.defaultRegion.region')) { + next(() => { + controller.set('region', null); }); - this._afterSetups = []; - }); + } + return this._super(...arguments); }, diff --git a/ui/app/routes/jobs.js b/ui/app/routes/jobs.js index a221735b6be..49564910684 100644 --- a/ui/app/routes/jobs.js +++ b/ui/app/routes/jobs.js @@ -1,5 +1,4 @@ import { inject as service } from '@ember/service'; -import { next } from '@ember/runloop'; import Route from '@ember/routing/route'; import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state'; import notifyForbidden from 'nomad-ui/utils/notify-forbidden'; @@ -21,39 +20,15 @@ export default Route.extend(WithForbiddenState, { }, }, - afterSetup(fn) { - this._afterSetups || (this._afterSetups = []); - this._afterSetups.push(fn); - }, - beforeModel(transition) { return this.get('system.namespaces').then(namespaces => { const queryParam = transition.queryParams.namespace; - const activeNamespace = this.get('system.activeNamespace.id'); - - if (!queryParam && activeNamespace !== 'default') { - this.afterSetup(controller => { - controller.set('jobNamespace', activeNamespace); - }); - } else if (queryParam && queryParam !== activeNamespace) { - this.set('system.activeNamespace', queryParam); - } + this.set('system.activeNamespace', queryParam || 'default'); return namespaces; }); }, - afterModel() { - const controller = this.controllerFor('jobs'); - next(() => { - (this._afterSetups || []).forEach(fn => { - fn(controller); - }); - this._afterSetups = []; - }); - return this._super(...arguments); - }, - model() { return this.get('store') .findAll('job', { reload: true }) diff --git a/ui/app/services/system.js b/ui/app/services/system.js index da7aa10cad5..7d6d2179243 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -58,9 +58,6 @@ export default Service.extend({ return region; } - // If the region in localStorage is no longer in the cluster, it needs to - // be cleared from localStorage - // this.set('activeRegion', null); return null; }, set(key, value) { @@ -114,7 +111,7 @@ export default Service.extend({ return namespace; } - // If the namespace is localStorage is no longer in the cluster, it needs to + // If the namespace in localStorage is no longer in the cluster, it needs to // be cleared from localStorage this.set('activeNamespace', null); return this.get('namespaces').findBy('id', 'default'); diff --git a/ui/app/templates/components/region-switcher.hbs b/ui/app/templates/components/region-switcher.hbs index 36e9ad1e2c0..df2da187093 100644 --- a/ui/app/templates/components/region-switcher.hbs +++ b/ui/app/templates/components/region-switcher.hbs @@ -1,6 +1,7 @@ {{#if system.shouldShowRegions}} {{#power-select data-test-region-switcher + tagName="div" triggerClass=decoration options=sortedRegions selected=system.activeRegion From c55df8ff4b6168879a08e589016355d376df9c95 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 9 Aug 2018 18:21:07 -0700 Subject: [PATCH 27/32] Specify the request type for token self Without this, the data (query params) get json stringified --- ui/app/adapters/token.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/app/adapters/token.js b/ui/app/adapters/token.js index 32315037ae7..8b8a8b25072 100644 --- a/ui/app/adapters/token.js +++ b/ui/app/adapters/token.js @@ -7,7 +7,7 @@ export default ApplicationAdapter.extend({ namespace: namespace + '/acl', findSelf() { - return this.ajax(`${this.buildURL()}/token/self`).then(token => { + return this.ajax(`${this.buildURL()}/token/self`, 'GET').then(token => { const store = this.get('store'); store.pushPayload('token', { tokens: [token], From dbc18c9d468460d5babba06edd1154f43d408393 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 9 Aug 2018 19:57:21 -0700 Subject: [PATCH 28/32] Properly model regions in Mirage --- ui/mirage/config.js | 7 ++++--- ui/mirage/factories/region.js | 7 +++++++ ui/mirage/models/region.js | 3 +++ ui/mirage/scenarios/default.js | 8 ++++++++ ui/mirage/serializers/region.js | 8 ++++++++ 5 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 ui/mirage/factories/region.js create mode 100644 ui/mirage/models/region.js create mode 100644 ui/mirage/serializers/region.js diff --git a/ui/mirage/config.js b/ui/mirage/config.js index fa7c1f6a1a1..9a419ba7157 100644 --- a/ui/mirage/config.js +++ b/ui/mirage/config.js @@ -165,8 +165,9 @@ export default function() { return new Response(501, {}, null); }); - this.get('/agent/members', function({ agents }) { + this.get('/agent/members', function({ agents, regions }) { return { + ServerRegion: regions.first().id, Members: this.serialize(agents.all()), }; }); @@ -222,8 +223,8 @@ export default function() { return new Response(403, {}, null); }); - this.get('/regions', function() { - return JSON.stringify(['global']); + this.get('/regions', function({ regions }) { + return this.serialize(regions.all()); }); const clientAllocationStatsHandler = function({ clientAllocationStats }, { params }) { diff --git a/ui/mirage/factories/region.js b/ui/mirage/factories/region.js new file mode 100644 index 00000000000..2802d161036 --- /dev/null +++ b/ui/mirage/factories/region.js @@ -0,0 +1,7 @@ +import { Factory } from 'ember-cli-mirage'; + +export default Factory.extend({ + id: () => { + throw new Error('The region factory will not generate IDs!'); + }, +}); diff --git a/ui/mirage/models/region.js b/ui/mirage/models/region.js new file mode 100644 index 00000000000..ddb04151dfb --- /dev/null +++ b/ui/mirage/models/region.js @@ -0,0 +1,3 @@ +import { Model } from 'ember-cli-mirage'; + +export default Model.extend(); diff --git a/ui/mirage/scenarios/default.js b/ui/mirage/scenarios/default.js index e40dee2cb3d..d80b5d276ea 100644 --- a/ui/mirage/scenarios/default.js +++ b/ui/mirage/scenarios/default.js @@ -2,6 +2,7 @@ import config from 'nomad-ui/config/environment'; const withNamespaces = getConfigValue('mirageWithNamespaces', false); const withTokens = getConfigValue('mirageWithTokens', true); +const withRegions = getConfigValue('mirageWithRegions', false); const allScenarios = { smallCluster, @@ -27,6 +28,7 @@ export default function(server) { if (withNamespaces) createNamespaces(server); if (withTokens) createTokens(server); + if (withRegions) createRegions(server); activeScenario(server); } @@ -98,6 +100,12 @@ function createNamespaces(server) { server.createList('namespace', 3); } +function createRegions(server) { + ['americas', 'europe', 'asia', 'some-long-name-just-to-test'].forEach(id => { + server.create('region', { id }); + }); +} + /* eslint-disable */ function logTokens(server) { console.log('TOKENS:'); diff --git a/ui/mirage/serializers/region.js b/ui/mirage/serializers/region.js new file mode 100644 index 00000000000..f38a82674c4 --- /dev/null +++ b/ui/mirage/serializers/region.js @@ -0,0 +1,8 @@ +import ApplicationSerializer from './application'; + +export default ApplicationSerializer.extend({ + serialize() { + var json = ApplicationSerializer.prototype.serialize.apply(this, arguments); + return [].concat(json).mapBy('ID'); + }, +}); From ae464a01dcf047d4a05a8e01d1125ed5abb6ecd8 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 9 Aug 2018 20:27:06 -0700 Subject: [PATCH 29/32] Unit test coverage for adding the region param to requests --- ui/tests/unit/adapters/job-test.js | 62 ++++++++++++++++++++++++++++ ui/tests/unit/services/token-test.js | 59 ++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 ui/tests/unit/services/token-test.js diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index 9b091c33e1a..0a1fb9f657d 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -40,6 +40,9 @@ moduleForAdapter('job', 'Unit | Adapter | Job', { this.server.create('job', { id: 'job-1', namespaceId: 'default' }); this.server.create('job', { id: 'job-2', namespaceId: 'some-namespace' }); + this.server.create('region', { id: 'region-1' }); + this.server.create('region', { id: 'region-2' }); + this.system = getOwner(this).lookup('service:system'); // Namespace, default region, and all regions are requests that all @@ -391,6 +394,65 @@ test('canceling a find record request will never cancel a request with the same }); }); +test('when there is no region set, requests are made without the region query param', function(assert) { + const { pretender } = this.server; + const jobName = 'job-1'; + const jobNamespace = 'default'; + const jobId = JSON.stringify([jobName, jobNamespace]); + + return wait().then(() => { + this.subject().findRecord(null, { modelName: 'job' }, jobId); + this.subject().findAll(null, { modelName: 'job' }, null); + + assert.deepEqual( + pretender.handledRequests.mapBy('url'), + [`/v1/job/${jobName}`, '/v1/jobs'], + 'No requests include the region query param' + ); + }); +}); + +test('when there is a region set, requests are made with the region query param', function(assert) { + const region = 'region-2'; + window.localStorage.nomadActiveRegion = region; + + const { pretender } = this.server; + const jobName = 'job-1'; + const jobNamespace = 'default'; + const jobId = JSON.stringify([jobName, jobNamespace]); + + return wait().then(() => { + this.subject().findRecord(null, { modelName: 'job' }, jobId); + this.subject().findAll(null, { modelName: 'job' }, null); + + assert.deepEqual( + pretender.handledRequests.mapBy('url'), + [`/v1/job/${jobName}?region=${region}`, `/v1/jobs?region=${region}`], + 'Requests include the region query param' + ); + }); +}); + +test('when the region is set to the default region, requests are made without the region query param', function(assert) { + window.localStorage.nomadActiveRegion = 'region-1'; + + const { pretender } = this.server; + const jobName = 'job-1'; + const jobNamespace = 'default'; + const jobId = JSON.stringify([jobName, jobNamespace]); + + return wait().then(() => { + this.subject().findRecord(null, { modelName: 'job' }, jobId); + this.subject().findAll(null, { modelName: 'job' }, null); + + assert.deepEqual( + pretender.handledRequests.mapBy('url'), + [`/v1/job/${jobName}`, '/v1/jobs'], + 'No requests include the region query param' + ); + }); +}); + function makeMockModel(id, options) { return assign( { diff --git a/ui/tests/unit/services/token-test.js b/ui/tests/unit/services/token-test.js new file mode 100644 index 00000000000..ed9d6f2e97c --- /dev/null +++ b/ui/tests/unit/services/token-test.js @@ -0,0 +1,59 @@ +import { getOwner } from '@ember/application'; +import Service from '@ember/service'; +import { moduleFor, test } from 'ember-qunit'; +import Pretender from 'pretender'; + +moduleFor('service:token', 'Unit | Service | Token', { + beforeEach() { + const mockSystem = Service.extend({ + activeRegion: 'region-1', + shouldIncludeRegion: true, + }); + + this.register('service:system', mockSystem); + this.system = getOwner(this).lookup('service:system'); + + this.server = new Pretender(function() { + this.get('/path', () => [200, {}, null]); + }); + }, + afterEach() { + this.server.shutdown(); + }, + subject() { + return getOwner(this) + .factoryFor('service:token') + .create(); + }, +}); + +test('authorizedRequest includes the region param when the system service says to', function(assert) { + const token = this.subject(); + + token.authorizedRequest('/path'); + assert.equal( + this.server.handledRequests.pop().url, + `/path?region=${this.system.get('activeRegion')}`, + 'The region param is included when the system service shouldIncludeRegion property is true' + ); + + this.system.set('shouldIncludeRegion', false); + + token.authorizedRequest('/path'); + assert.equal( + this.server.handledRequests.pop().url, + '/path', + 'The region param is not included when the system service shouldIncludeRegion property is false' + ); +}); + +test('authorizedRawRequest bypasses adding the region param', function(assert) { + const token = this.subject(); + + token.authorizedRawRequest('/path'); + assert.equal( + this.server.handledRequests.pop().url, + '/path', + 'The region param is ommitted when making a raw request' + ); +}); From 95e8259447792b04f4c2d10691b8f395186e5f71 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Fri, 10 Aug 2018 11:20:44 -0700 Subject: [PATCH 30/32] Acceptance tests for the region switcher --- ui/app/components/global-header.js | 2 + ui/app/templates/components/gutter-menu.hbs | 6 +- ui/app/templates/jobs/index.hbs | 2 +- ui/mirage/config.js | 3 +- ui/tests/acceptance/regions-test.js | 210 ++++++++++++++++++++ ui/tests/helpers/module-for-acceptance.js | 2 +- ui/tests/pages/jobs/list.js | 1 + ui/tests/pages/layout.js | 31 +++ 8 files changed, 251 insertions(+), 6 deletions(-) create mode 100644 ui/tests/acceptance/regions-test.js create mode 100644 ui/tests/pages/layout.js diff --git a/ui/app/components/global-header.js b/ui/app/components/global-header.js index 6ee147b08e7..c90e6457dfd 100644 --- a/ui/app/components/global-header.js +++ b/ui/app/components/global-header.js @@ -1,5 +1,7 @@ import Component from '@ember/component'; export default Component.extend({ + 'data-test-global-header': true, + onHamburgerClick() {}, }); diff --git a/ui/app/templates/components/gutter-menu.hbs b/ui/app/templates/components/gutter-menu.hbs index 8e0a8ab0ce7..4c182af6f74 100644 --- a/ui/app/templates/components/gutter-menu.hbs +++ b/ui/app/templates/components/gutter-menu.hbs @@ -49,14 +49,14 @@
{{/if}} -
  • {{#link-to "jobs" activeClass="is-active"}}Jobs{{/link-to}}
    • +
  • {{#link-to "jobs" activeClass="is-active" data-test-gutter-link="jobs"}}Jobs{{/link-to}}

    • Cluster

      -
    • {{#link-to "clients" activeClass="is-active"}}Clients{{/link-to}}
      • -
    • {{#link-to "servers" activeClass="is-active"}}Servers{{/link-to}}
      • +
    • {{#link-to "clients" activeClass="is-active" data-test-gutter-link="clients"}}Clients{{/link-to}}
      • +
    • {{#link-to "servers" activeClass="is-active" data-test-gutter-link="servers"}}Servers{{/link-to}}
    diff --git a/ui/app/templates/jobs/index.hbs b/ui/app/templates/jobs/index.hbs index 3985b9f0008..a306f510616 100644 --- a/ui/app/templates/jobs/index.hbs +++ b/ui/app/templates/jobs/index.hbs @@ -25,7 +25,7 @@ Summary {{/t.head}} {{#t.body key="model.id" as |row|}} - {{job-row data-test-job-row job=row.model onClick=(action "gotoJob" row.model)}} + {{job-row data-test-job-row=row.model.plainId job=row.model onClick=(action "gotoJob" row.model)}} {{/t.body}} {{/list-table}}
    diff --git a/ui/mirage/config.js b/ui/mirage/config.js index 9a419ba7157..0311a8d94eb 100644 --- a/ui/mirage/config.js +++ b/ui/mirage/config.js @@ -166,8 +166,9 @@ export default function() { }); this.get('/agent/members', function({ agents, regions }) { + const firstRegion = regions.first(); return { - ServerRegion: regions.first().id, + ServerRegion: firstRegion ? firstRegion.id : null, Members: this.serialize(agents.all()), }; }); diff --git a/ui/tests/acceptance/regions-test.js b/ui/tests/acceptance/regions-test.js new file mode 100644 index 00000000000..9fae4d6a3f6 --- /dev/null +++ b/ui/tests/acceptance/regions-test.js @@ -0,0 +1,210 @@ +import { test } from 'qunit'; +import moduleForAcceptance from 'nomad-ui/tests/helpers/module-for-acceptance'; +import JobsList from 'nomad-ui/tests/pages/jobs/list'; +import ClientsList from 'nomad-ui/tests/pages/clients/list'; +import PageLayout from 'nomad-ui/tests/pages/layout'; +import Allocation from 'nomad-ui/tests/pages/allocations/detail'; + +moduleForAcceptance('Acceptance | regions (only one)', { + beforeEach() { + server.create('agent'); + server.create('node'); + server.createList('job', 5); + }, +}); + +test('when there is only one region, the region switcher is not shown in the nav bar', function(assert) { + server.create('region', { id: 'global' }); + + andThen(() => { + JobsList.visit(); + }); + + andThen(() => { + assert.notOk(PageLayout.navbar.regionSwitcher.isPresent, 'No region switcher'); + }); +}); + +test('when the only region is not named "global", the region switcher still is not shown', function(assert) { + server.create('region', { id: 'some-region' }); + + andThen(() => { + JobsList.visit(); + }); + + andThen(() => { + assert.notOk(PageLayout.navbar.regionSwitcher.isPresent, 'No region switcher'); + }); +}); + +test('pages do not include the region query param', function(assert) { + let jobId; + + server.create('region', { id: 'global' }); + + andThen(() => { + JobsList.visit(); + }); + andThen(() => { + assert.equal(currentURL(), '/jobs', 'No region query param'); + }); + andThen(() => { + jobId = JobsList.jobs.objectAt(0).id; + JobsList.jobs.objectAt(0).clickRow(); + }); + andThen(() => { + assert.equal(currentURL(), `/jobs/${jobId}`, 'No region query param'); + }); + andThen(() => { + ClientsList.visit(); + }); + andThen(() => { + assert.equal(currentURL(), '/clients', 'No region query param'); + }); +}); + +test('api requests do not include the region query param', function(assert) { + server.create('region', { id: 'global' }); + + andThen(() => { + JobsList.visit(); + }); + andThen(() => { + JobsList.jobs.objectAt(0).clickRow(); + }); + andThen(() => { + PageLayout.gutter.visitClients(); + }); + andThen(() => { + PageLayout.gutter.visitServers(); + }); + andThen(() => { + server.pretender.handledRequests.forEach(req => { + assert.notOk(req.url.includes('region='), req.url); + }); + }); +}); + +moduleForAcceptance('Acceptance | regions (many)', { + beforeEach() { + server.create('agent'); + server.create('node'); + server.createList('job', 5); + server.create('region', { id: 'global' }); + server.create('region', { id: 'region-2' }); + }, +}); + +test('the region switcher is rendered in the nav bar', function(assert) { + JobsList.visit(); + + andThen(() => { + assert.ok(PageLayout.navbar.regionSwitcher.isPresent, 'Region switcher is shown'); + }); +}); + +test('when on the default region, pages do not include the region query param', function(assert) { + JobsList.visit(); + + andThen(() => { + assert.equal(currentURL(), '/jobs', 'No region query param'); + assert.equal(window.localStorage.nomadActiveRegion, 'global', 'Region in localStorage'); + }); +}); + +test('switching regions sets localStorage and the region query param', function(assert) { + const newRegion = server.db.regions[1].id; + + JobsList.visit(); + + selectChoose('[data-test-region-switcher]', newRegion); + + andThen(() => { + assert.ok( + currentURL().includes(`region=${newRegion}`), + 'New region is the region query param value' + ); + assert.equal(window.localStorage.nomadActiveRegion, newRegion, 'New region in localStorage'); + }); +}); + +test('switching regions to the default region, unsets the region query param', function(assert) { + const startingRegion = server.db.regions[1].id; + const defaultRegion = server.db.regions[0].id; + + JobsList.visit({ region: startingRegion }); + + selectChoose('[data-test-region-switcher]', defaultRegion); + + andThen(() => { + assert.equal(currentURL(), '/jobs', 'No region query param for the default region'); + assert.equal( + window.localStorage.nomadActiveRegion, + defaultRegion, + 'New region in localStorage' + ); + }); +}); + +test('switching regions on deep pages redirects to the application root', function(assert) { + const newRegion = server.db.regions[1].id; + + Allocation.visit({ id: server.db.allocations[0].id }); + + selectChoose('[data-test-region-switcher]', newRegion); + + andThen(() => { + assert.ok(currentURL().includes('/jobs?'), 'Back at the jobs page'); + }); +}); + +test('navigating directly to a page with the region query param sets the application to that region', function(assert) { + const allocation = server.db.allocations[0]; + const region = server.db.regions[1].id; + Allocation.visit({ id: allocation.id, region }); + + andThen(() => { + assert.equal( + currentURL(), + `/allocations/${allocation.id}?region=${region}`, + 'Region param is persisted when navigating straight to a detail page' + ); + assert.equal( + window.localStorage.nomadActiveRegion, + region, + 'Region is also set in localStorage from a detail page' + ); + }); +}); + +test('when the region is not the default region, all api requests include the region query param', function(assert) { + const region = server.db.regions[1].id; + + JobsList.visit({ region }); + + andThen(() => { + JobsList.jobs.objectAt(0).clickRow(); + }); + andThen(() => { + PageLayout.gutter.visitClients(); + }); + andThen(() => { + PageLayout.gutter.visitServers(); + }); + andThen(() => { + const [regionsRequest, defaultRegionRequest, ...appRequests] = server.pretender.handledRequests; + + assert.notOk( + regionsRequest.url.includes('region='), + 'The regions request is made without a region qp' + ); + assert.notOk( + defaultRegionRequest.url.includes('region='), + 'The default region request is made without a region qp' + ); + + appRequests.forEach(req => { + assert.ok(req.url.includes(`region=${region}`), req.url); + }); + }); +}); diff --git a/ui/tests/helpers/module-for-acceptance.js b/ui/tests/helpers/module-for-acceptance.js index 44fe4e0e2f4..05110ce6d24 100644 --- a/ui/tests/helpers/module-for-acceptance.js +++ b/ui/tests/helpers/module-for-acceptance.js @@ -9,7 +9,7 @@ export default function(name, options = {}) { // Clear session storage (a side effect of token storage) window.sessionStorage.clear(); - // Also clear local storage (a side effect of namespaces) + // Also clear local storage (a side effect of namespaces and regions) window.localStorage.clear(); this.application = startApp(); diff --git a/ui/tests/pages/jobs/list.js b/ui/tests/pages/jobs/list.js index fa05760d93f..cd59ac7ef67 100644 --- a/ui/tests/pages/jobs/list.js +++ b/ui/tests/pages/jobs/list.js @@ -17,6 +17,7 @@ export default create({ search: fillable('[data-test-jobs-search] input'), jobs: collection('[data-test-job-row]', { + id: attribute('data-test-job-row'), name: text('[data-test-job-name]'), link: attribute('href', '[data-test-job-name] a'), status: text('[data-test-job-status]'), diff --git a/ui/tests/pages/layout.js b/ui/tests/pages/layout.js new file mode 100644 index 00000000000..e276310bb14 --- /dev/null +++ b/ui/tests/pages/layout.js @@ -0,0 +1,31 @@ +import { create, clickable, collection, isPresent, text } from 'ember-cli-page-object'; + +export default create({ + navbar: { + scope: '[data-test-global-header]', + + regionSwitcher: { + scope: '[data-test-region-switcher]', + isPresent: isPresent(), + open: clickable('.ember-power-select-trigger'), + options: collection('.ember-power-select-option', { + label: text(), + }), + }, + }, + + gutter: { + scope: '[data-test-gutter-menu]', + namespaceSwitcher: { + scope: '[data-test-namespace-switcher]', + isPresent: isPresent(), + open: clickable('.ember-power-select-trigger'), + options: collection('.ember-power-select-option', { + label: text(), + }), + }, + visitJobs: clickable('[data-test-gutter-link="jobs"]'), + visitClients: clickable('[data-test-gutter-link="clients"]'), + visitServers: clickable('[data-test-gutter-link="servers"]'), + }, +}); From 31922670393abb0334391d99f129edf39a88488e Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 13 Aug 2018 16:18:06 -0700 Subject: [PATCH 31/32] Use the model hook and setupController hook instead of afterModel This is a more idiomatic way to handle the QP resetting in the application controller. --- ui/app/routes/application.js | 15 ++++++++------- ui/tests/acceptance/regions-test.js | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index 6119eb4cea4..477d30c49d3 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -46,14 +46,15 @@ export default Route.extend({ ); }, - // setupController doesn't refire when the model hook refires as part of - // a query param change - afterModel(model, transition) { - const queryParam = transition.queryParams.region; - const controller = this.controllerFor('application'); + // Model is being used as a way to transfer the provided region + // query param to update the controller state. + model(params) { + return params.region; + }, + + setupController(controller, model) { + const queryParam = model; - // The default region shouldn't show up as a query param since - // it's superfluous. if (queryParam === this.get('system.defaultRegion.region')) { next(() => { controller.set('region', null); diff --git a/ui/tests/acceptance/regions-test.js b/ui/tests/acceptance/regions-test.js index 9fae4d6a3f6..2a392eed13c 100644 --- a/ui/tests/acceptance/regions-test.js +++ b/ui/tests/acceptance/regions-test.js @@ -137,7 +137,7 @@ test('switching regions to the default region, unsets the region query param', f selectChoose('[data-test-region-switcher]', defaultRegion); andThen(() => { - assert.equal(currentURL(), '/jobs', 'No region query param for the default region'); + assert.notOk(currentURL().includes('region='), 'No region query param for the default region'); assert.equal( window.localStorage.nomadActiveRegion, defaultRegion, From a61ad7ac59a158a7ec7c852b051a931f39d98dd3 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 13 Aug 2018 16:18:53 -0700 Subject: [PATCH 32/32] List the new region mirage env var in the environment file --- ui/config/environment.js | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/config/environment.js b/ui/config/environment.js index 3fafff3245c..2c8f0c688e6 100644 --- a/ui/config/environment.js +++ b/ui/config/environment.js @@ -23,6 +23,7 @@ module.exports = function(environment) { mirageScenario: 'smallCluster', mirageWithNamespaces: false, mirageWithTokens: true, + mirageWithRegions: true, }, };