Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Adds blocking query support to the service detail page #5479

Merged
merged 1 commit into from
Mar 22, 2019

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Mar 12, 2019

This PR adds blocking query support to the new Service Detail page (new in that it now lists Service Instances instead of Nodes, and lets you click into the Instances to visit a new third level/detail page)

Additionally here we always wanted to provide functionality to warn the user of the deregistration of a Service if you are on the Service Detail page at the time of deregistration. This PR includes functionality via blocking queries to detect Service deregistration and show a warning message to the user, but also keep the old data visible incase the user still needs the information on the Service.

A visual walkthrough of this can be seen in the GIF here #5070 (comment) on deletion of the cdn Service (the page is now slightly different due to the Service Instance work)

It essentially looks like this if you are viewing a service as it is deregistered:

Screenshot 2019-03-12 at 18 12 18

Notes:

Meta Data

In #4946 we used JSON-API's meta data for X-Consul-Index. It turns out that ember-data only supports JSON-API meta data usage on collections currently, not on single records/responses. There are various GH issues around that discuss this, so it seems to be a fairly common issue.

Instead of doing anything too complicated, we simply added a meta property to the Service model.

An interesting 'saviour' here is related to the fact that the Consul API uses capital case for all its properties. A lot of Consul entities/nouns/models already have a Meta property, so it seems like due to a piece of historical pragmatism/accident we were able to have both a Meta property, which is actual Consul meta data, and also a meta property which is more temporary JSON-API meta data (not used within templates).

Despite the fact that capital case feels a little strange in JS land, we've always liked having the capital case API data as its immediately apparent what's coming from the API and what isn't, which is especially useful whilst editing templates. But now, this fact is enabling us to add functionality without having to jump through too many hoops.

Computed Properties / listen / setProperties

In order to achieve a lot of what we are doing here 'transparently' (so without having to delve too much in amending templates). We've used a lot of work from #5079 to prevent us from having to change the names of view properties/data. So for example, we continue to use item/items for names whilst taking advantage of the extra error listening functionality provided by EventSources. We continue to use setProperties here also and there is some information on the ins and outs of that here #4789 (comment), and I'm still hoping to move away from using setProperties in the future and replace it with our own custom 'I'm ready to render now' method. Oh lastly here, thanks @meirish for helping me to figure out the correct usage of metaForProperty here.

Misc

  • We added a 'placeholder' compile time configuration property here (CONSUL_UI_DISABLE_REALTIME) to globally disable blocking queries, this probably should have gone in a past PR.
  • I'm really getting to the point now where it would be better to have some of this re-shaping in the serializer where it would then make sense to use the ember-data specific errors rather than a default javascript error (reasons here: we currently try to keep any ember-data imports out of the repositories and only use the provided ember-data APIs). This currently works fine but I guess it would be best for it to throw an ember-data error.
  • We'll probably change consul-api-double at some point to talk about 'instances' rather than 'nodes' its not really required at the moment though so we've done whats needed here instead ('instances' equals CONSUL_NODE_COUNT)
  • I'm still quibbling with myself on the use of .catch as a name for for the error catching method. At some point there will be a usecase to have other events like add, delete etc to provide finer grained functionality for specific types or property changes rather than just 'change' - the word catch probably wouldn't work as well here. Its fine for the moment so I left as is.

Lastly, there'll be very similar yet smaller PRs coming soon for the Nodes detail page and the Service Instance Detail page, which will take the exact same approach here, but I left these out of the PR so its easier to digest.

Another implementation of this for the service instance detail page is in a PR here: #5487

This commit includes several pieces of functionality to enable services
to be removed and the page to present information that this has happened
but also keep the deleted information on the page. Along with the more
usual blocking query based listing.

To enable this:

1. Implements `meta` on the model (only available on collections in
ember)
2. Adds new `catchable` ComputedProperty alongside a `listen` helper for
working with specific errors that can be thrown from EventSources in an
ember-like way. Briefly, normal computed properties update when a
property changes, EventSources can additionally throw errors so we can
catch them and show different visuals based on that.
Copy link

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

The deregistered flash message is a nice touch.

@johncowen johncowen merged commit 424c834 into ui-staging Mar 22, 2019
@johncowen johncowen deleted the feature/ui-instance-blocking branch April 8, 2019 10:30
johncowen added a commit that referenced this pull request Apr 29, 2019
This commit includes several pieces of functionality to enable services
to be removed and the page to present information that this has happened
but also keep the deleted information on the page. Along with the more
usual blocking query based listing.

To enable this:

1. Implements `meta` on the model (only available on collections in
ember)
2. Adds new `catchable` ComputedProperty alongside a `listen` helper for
working with specific errors that can be thrown from EventSources in an
ember-like way. Briefly, normal computed properties update when a
property changes, EventSources can additionally throw errors so we can
catch them and show different visuals based on that.
johncowen added a commit that referenced this pull request May 1, 2019
This commit includes several pieces of functionality to enable services
to be removed and the page to present information that this has happened
but also keep the deleted information on the page. Along with the more
usual blocking query based listing.

To enable this:

1. Implements `meta` on the model (only available on collections in
ember)
2. Adds new `catchable` ComputedProperty alongside a `listen` helper for
working with specific errors that can be thrown from EventSources in an
ember-like way. Briefly, normal computed properties update when a
property changes, EventSources can additionally throw errors so we can
catch them and show different visuals based on that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants