Skip to content

Commit

Permalink
ui: Address some Admin Partition FIXMEs
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
John Cowen committed Sep 24, 2021
1 parent 94d3849 commit 315e23c
Show file tree
Hide file tree
Showing 10 changed files with 27 additions and 14 deletions.
3 changes: 1 addition & 2 deletions ui/packages/consul-ui/app/adapters/partition.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand All @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion ui/packages/consul-ui/app/components/auth-form/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
focus=(action 'focus')
)}}
<Guard @name="hasValue" @cond={{action 'hasValue'}} />
{{!FIXME: Call this reset or similar }}
{{!TODO: Call this reset or similar }}
<Action @name="clearError" @exec={{queue (action (mut error) undefined) (action (mut secret) undefined)}} />
<div class="auth-form" ...attributes>
<State @matches="error">
Expand Down
11 changes: 9 additions & 2 deletions ui/packages/consul-ui/app/components/data-source/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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' &&
Expand Down Expand Up @@ -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([]);
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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|}}
<MenuItem
class={{if (eq @partition item.Name) 'is-active'}}
Expand Down
2 changes: 1 addition & 1 deletion ui/packages/consul-ui/app/components/token-source/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export default Component.extend({
},
};
};
// FIXME: We should probably put the component into idle state
// TODO: We should probably put the component into idle state
this.onchange(e);
},
},
Expand Down
4 changes: 1 addition & 3 deletions ui/packages/consul-ui/app/models/service-instance.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ export default class ServiceInstance extends Model {
@alias('Service.Tags') Tags;
@alias('Service.Meta') Meta;
@alias('Service.Namespace') Namespace;

// FIXME: Is parition top level or Service.Partition?
@attr('string') Partition;
@alias('Service.Partition') Partition;

@filter('Checks.@each.Kind', (item, i, arr) => item.Kind === 'service') ServiceChecks;
@filter('Checks.@each.Kind', (item, i, arr) => item.Kind === 'node') NodeChecks;
Expand Down
5 changes: 4 additions & 1 deletion ui/packages/consul-ui/app/routing/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
4 changes: 2 additions & 2 deletions ui/packages/consul-ui/app/services/repository/dc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ as |route|>
@filter={{filters}}
/>
{{/if}}
{{!FIXME: Check param order }}
<DataWriter
@sink={{uri
'/${partition}/${dc}/${nspace}/intention/'
Expand Down

0 comments on commit 315e23c

Please sign in to comment.