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

Hedge for when consul sends nodes with an empty ID #4331

Merged
merged 2 commits into from
Jul 3, 2018

Conversation

johncowen
Copy link
Contributor

Addresses #4305 and #4249 also see #4294

This uses the node Name as the unique 'slug' if ID is blank

@johncowen johncowen requested review from a team and mkeeler July 3, 2018 12:30
I don't think this would have a large effect on the UI whichever but
best to make sure
@@ -1,6 +1,14 @@
import Adapter from './application';
import { PRIMARY_KEY, SLUG_KEY } from 'consul-ui/models/node';
import { OK as HTTP_OK } from 'consul-ui/utils/http/status';
// TODO: Looks like ID just isn't used at all
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean that ID isn't used at all here? It looks like you're generating IDs on the client and using that as the primary key (uid) so ember data is never using the ID from the server for its identity map. Probably not an issue as long as you're not caching the list or editing things, but something to be aware of if you ever want to add that sort of thing. I'm guessing you already work around that though for individual node pages (or you fetch the group first).

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, yeah its more or less a note to go back and consider just ignoring ID completely, and just pretend Node is the 'unique key' to use for the slug. It sounds like that is always unique and ID can sometimes be empty, although, even while I'm writing this, the more i think about it the more I'd just continue to use ID - it just reads better plus the big has been fixed in the backend now - this is just for old data I think.

Copy link
Collaborator

@meirish meirish left a comment

Choose a reason for hiding this comment

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

This is more just in case right? The linked PR should now be generating a UUID on the backend, yeah?

Either way, code looks good! Had one comment that was more observation than anything. 👍

@johncowen
Copy link
Contributor Author

Yup, mkeeler fixed a bug in the backend where you'd end up with nodes with no ID's, this is to cope for older data, when I can still end up in the UI with nodes with no ID's

Cheers!

@johncowen johncowen merged commit 87a0ad9 into master Jul 3, 2018
@johncowen johncowen deleted the feature/hedge-empty-node-ids branch July 3, 2018 15:11
@johncowen johncowen added the theme/ui Anything related to the UI label Nov 29, 2019
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