From 1b9586b65ea2b38d69f0cedf782abfc48572dec2 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Fri, 1 Oct 2021 11:07:58 +0100 Subject: [PATCH] ui: Address some Admin Partition FIXMEs (#11057) This commit addresses some left over admin partition FIXMEs 1. Adds Partition correctly to Service Instances 2. Converts non-important 'we can do this later' FIXMEs to TODOs 3. Removes some FIXMEs that I've double checked and addressed. Most of the remaining FIXMEs I'm waiting on responses to questions from the consul core folks for. I'll address those in a separate PR. --- ui/packages/consul-ui/app/adapters/partition.js | 3 +-- .../consul-ui/app/components/auth-form/index.hbs | 2 +- .../consul-ui/app/components/data-source/index.js | 11 +++++++++-- .../app/components/hashicorp-consul/index.hbs | 1 - .../consul-ui/app/components/token-source/index.js | 2 +- ui/packages/consul-ui/app/models/service-instance.js | 4 +--- ui/packages/consul-ui/app/routing/route.js | 5 ++++- .../app/services/data-sink/protocols/http.js | 8 ++++++++ ui/packages/consul-ui/app/services/repository/dc.js | 4 ++-- .../templates/dc/services/show/intentions/index.hbs | 1 - 10 files changed, 27 insertions(+), 14 deletions(-) diff --git a/ui/packages/consul-ui/app/adapters/partition.js b/ui/packages/consul-ui/app/adapters/partition.js index bbcdd3a72..542001a00 100644 --- a/ui/packages/consul-ui/app/adapters/partition.js +++ b/ui/packages/consul-ui/app/adapters/partition.js @@ -2,7 +2,6 @@ import Adapter from './application'; // Blocking query support for partitions is currently disabled export default class PartitionAdapter extends Adapter { - // FIXME: Check overall hierarchy again async requestForQuery(request, { ns, dc, index }) { const respond = await request` GET /v1/partitions?${{ dc }} @@ -12,7 +11,7 @@ export default class PartitionAdapter extends Adapter { await respond((headers, body) => delete headers['x-consul-index']); return respond; } - + // TODO: Not used until we do Partition CRUD async requestForQueryRecord(request, { ns, dc, index, id }) { if (typeof id === 'undefined') { throw new Error('You must specify an id'); diff --git a/ui/packages/consul-ui/app/components/auth-form/index.hbs b/ui/packages/consul-ui/app/components/auth-form/index.hbs index 518f0c0e4..9379b0ade 100644 --- a/ui/packages/consul-ui/app/components/auth-form/index.hbs +++ b/ui/packages/consul-ui/app/components/auth-form/index.hbs @@ -4,7 +4,7 @@ focus=(action 'focus') )}} - {{!FIXME: Call this reset or similar }} + {{!TODO: Call this reset or similar }}
diff --git a/ui/packages/consul-ui/app/components/data-source/index.js b/ui/packages/consul-ui/app/components/data-source/index.js index 60e4b1db7..f854cc9f0 100644 --- a/ui/packages/consul-ui/app/components/data-source/index.js +++ b/ui/packages/consul-ui/app/components/data-source/index.js @@ -3,6 +3,7 @@ import { inject as service } from '@ember/service'; import { tracked } from '@glimmer/tracking'; import { action, get } from '@ember/object'; import { schedule } from '@ember/runloop'; +import { runInDebug } from '@ember/debug'; /** * Utility function to set, but actually replace if we should replace @@ -89,7 +90,8 @@ export default class DataSource extends Component { @action disconnect() { - // FIXME? Should we be doing this here + // TODO: Should we be doing this here? Fairly sure we should be so if this + // TODO gets old enough (6 months/ 1 year or so) feel free to remove if ( typeof this.data !== 'undefined' && typeof this.data.length === 'undefined' && @@ -182,7 +184,12 @@ export default class DataSource extends Component { this.source.readyState = 2; this.disconnect(); schedule('afterRender', () => { - // FIXME: Lazy data-sources + // TODO: Support lazy data-sources by keeping a reference to $el + runInDebug(_ => + console.error( + `Invalidation is only supported for non-lazy data sources. If you want to use this you should fixup support for lazy data sources` + ) + ); this.connect([]); }); } diff --git a/ui/packages/consul-ui/app/components/hashicorp-consul/index.hbs b/ui/packages/consul-ui/app/components/hashicorp-consul/index.hbs index b0c2a5f9c..78efefa5a 100644 --- a/ui/packages/consul-ui/app/components/hashicorp-consul/index.hbs +++ b/ui/packages/consul-ui/app/components/hashicorp-consul/index.hbs @@ -72,7 +72,6 @@ }} @onchange={{action (mut this.partitions) value="data"}} /> - {{!FIXME: Do partitions do the same as namespace deletion? }} {{#each (reject-by 'DeletedAt' this.partitions) as |item|}} item.Kind === 'service') ServiceChecks; @filter('Checks.@each.Kind', (item, i, arr) => item.Kind === 'node') NodeChecks; diff --git a/ui/packages/consul-ui/app/routing/route.js b/ui/packages/consul-ui/app/routing/route.js index 9ec09a47b..b36b2c0a6 100644 --- a/ui/packages/consul-ui/app/routing/route.js +++ b/ui/packages/consul-ui/app/routing/route.js @@ -84,7 +84,10 @@ export default class BaseRoute extends Route { return value; } - // FIXME: this is only required due to intention_id trying to do too much + // TODO: this is only required due to intention_id trying to do too much + // therefore we need to change the route parameter intention_id to just + // intention or id or similar then we can revert to only returning a model if + // we have searchProps (or a child route overwrites model) model() { const model = {}; if ( diff --git a/ui/packages/consul-ui/app/services/data-sink/protocols/http.js b/ui/packages/consul-ui/app/services/data-sink/protocols/http.js index 724b686a9..c6a293c2d 100644 --- a/ui/packages/consul-ui/app/services/data-sink/protocols/http.js +++ b/ui/packages/consul-ui/app/services/data-sink/protocols/http.js @@ -11,6 +11,14 @@ export default class HttpService extends Service { return setProperties(instance, data); } + // TODO: Currently we don't use the other properties here So dc, nspace and + // partition, but confusingly they currently are in a different order to all + // our @dataSource uris @dataSource uses /:partition/:nspace/:dc/thing whilst + // here DataSink uses /:parition/:dc/:nspace/thing We should change DataSink + // to also use a @dataSink decorator and make sure the order of the parameters + // is the same throughout the app As it stands right now, if we do need to use + // those parameters for DataSink it will be very easy to introduce a bug due + // to this inconsistency persist(sink, instance) { const [, , , , model] = sink.split('/'); const repo = this[model]; diff --git a/ui/packages/consul-ui/app/services/repository/dc.js b/ui/packages/consul-ui/app/services/repository/dc.js index 28c195d76..9de4e4fa5 100644 --- a/ui/packages/consul-ui/app/services/repository/dc.js +++ b/ui/packages/consul-ui/app/services/repository/dc.js @@ -18,10 +18,10 @@ export default class DcService extends RepositoryService { const items = this.store.peekAll('dc'); const item = items.findBy('Name', params.name); if (typeof item === 'undefined') { - // FIXME: HTTPError + // TODO: We should use a HTTPError error here and remove all occurances of + // the custom shaped ember-data error throughout the app const e = new Error('Page not found'); e.status = '404'; - // FIXME: EDError throw { errors: [e] }; } return item; diff --git a/ui/packages/consul-ui/app/templates/dc/services/show/intentions/index.hbs b/ui/packages/consul-ui/app/templates/dc/services/show/intentions/index.hbs index 942536e30..63e9815b1 100644 --- a/ui/packages/consul-ui/app/templates/dc/services/show/intentions/index.hbs +++ b/ui/packages/consul-ui/app/templates/dc/services/show/intentions/index.hbs @@ -56,7 +56,6 @@ as |route|> @filter={{filters}} /> {{/if}} - {{!FIXME: Check param order }}