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: Leader icon for node listing view #6265

Merged
merged 8 commits into from
Aug 12, 2019
Merged

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Aug 2, 2019

This builds upon the leader icons that where added here #6021 but just ifed out temporarily.

It addresses one part of #5073 by showing the leader in the node listing using a star icon and tooltip for further clarification.

To note:

The endpoint we are using doesn't support blocking queries we had 3 choices here:

  1. Everytime a blocking query for nodes resolves, immediately request the leader information again
  2. Make a 'fake' blocking query for the status/leader endpoint on the frontend, by hardcoding a 'cursor' for this endpoint, and essentially make it poll.
  3. Only request the leader information when the user enters the node listing page.

After talking to other in the team a while ago we went with point 3 here. On the basis that the leader doesn't update too often (potentially why the endpoint doesn't support blocking queries).

Potentially there is still a little work to do here, in that we may need to add something to cope with the state of consul when waiting for leader election. I thought it best to get eyes on this as soon as possible, and we can add any extra for this case ontop of here (either more commits or a PR ontop of this)

Screenshot 2019-08-02 at 16 04 02

@johncowen johncowen requested a review from a team August 2, 2019 14:04
@johncowen johncowen added the theme/ui Anything related to the UI label Aug 2, 2019
@johncowen
Copy link
Contributor Author

Potentially there is still a little work to do here, in that we may need to add something to cope with the state of consul when waiting for leader election. I thought it best to get eyes on this as soon as possible, and we can add any extra for this case ontop of here (either more commits or a PR ontop of this)

Consul just responds with an empty string ("") when there is no elected leader so we added a straightforward test to cover that just to assert that nothing goes wrong with the UI. Visually we don't show anything extra for the moment when no leader has been elected.

const modelName = 'node';
export default RepositoryService.extend({
coordinates: service('repository/coordinate'),
getModelName: function() {
return modelName;
},
findByLeader: function(dc) {
const query = {
dc: dc,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dc: dc,
dc,

Copy link
Contributor Author

@johncowen johncowen Aug 8, 2019

Choose a reason for hiding this comment

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

Hey @gregone ,

Thanks for looking at this! I was thinking of merging without doing this suggestion, reasons being:

Most of the Consul UI codebase is ES5-y (think ember 2.18 timescales here, 2-3 years back), we generally don't do this kind of assignment (I think the only ES6-y thing we use is the Object.assign-like {...{}, ...{}} which was only because we preferred that to having to use the ember provided assign)

Other examples specifically in Repositories are:

.self(this.getModelName(), {
secret: secret,
dc: dc,
})

const query = {
id: node,
dc: dc,
};

const query = {
id: slug,
dc: dc,
};

const query = {
id: key,
dc: dc,

..but there are a few more around and about.

I can completely see where you are coming from, right now it's one of those consistency/context things. Once we get past ember 3.1* we'll think about moving up a notch, and if we do we'll probably skim the entire codebase in one go so everything is consistent.

Anyway, let me know what you think, happy to add this before merging if you feel strongly about it.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @gregone

We are going to try and get this into our next release, so I'm going to merge this without the suggestion, hope that's ok, thanks for reviewing!

@johncowen johncowen merged commit ca317df into ui-staging Aug 12, 2019
@johncowen johncowen deleted the feature/ui-leader-icon branch August 12, 2019 15:43
johncowen added a commit that referenced this pull request Aug 22, 2019
- yarn upgrade consul-api-double which includes `status/leader`
- add all the ember-data things required to call a new endpoint
- Pass the new leader variable through to the template
- use the new leader variable in the template to set a leader
- add acceptance testing to verify leaders are highlighted
- Change testing navigation/api requests to status/leader (on the node listing page, status/leader is now the last get request to
be called).
- Template whitespace commit (less indenting)
- adds a test to to assert no errors happen with an unelected leader
This was referenced Aug 22, 2019
johncowen added a commit that referenced this pull request Aug 29, 2019
- yarn upgrade consul-api-double which includes `status/leader`
- add all the ember-data things required to call a new endpoint
- Pass the new leader variable through to the template
- use the new leader variable in the template to set a leader
- add acceptance testing to verify leaders are highlighted
- Change testing navigation/api requests to status/leader (on the node listing page, status/leader is now the last get request to
be called).
- Template whitespace commit (less indenting)
- adds a test to to assert no errors happen with an unelected leader
johncowen added a commit that referenced this pull request Sep 4, 2019
- yarn upgrade consul-api-double which includes `status/leader`
- add all the ember-data things required to call a new endpoint
- Pass the new leader variable through to the template
- use the new leader variable in the template to set a leader
- add acceptance testing to verify leaders are highlighted
- Change testing navigation/api requests to status/leader (on the node listing page, status/leader is now the last get request to
be called).
- Template whitespace commit (less indenting)
- adds a test to to assert no errors happen with an unelected leader
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