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

Remove kubernetes_id column from the grid view. #1774

Merged
merged 2 commits into from
Aug 11, 2016

Conversation

foot
Copy link
Contributor

@foot foot commented Aug 9, 2016

Redundant info

Fixes #1759

@davkal
Copy link
Contributor

davkal commented Aug 9, 2016

Wouldnt it make more sense not to include the field in the backend report in the first place?

@foot
Copy link
Contributor Author

foot commented Aug 9, 2016

Not sure, ids can often be handy though. I'll have a quick look.

@davkal
Copy link
Contributor

davkal commented Aug 9, 2016

Code works.

Not sure, ids can often be handy though. I'll have a quick look.

The original ticket said the field has useless information. So why not drop the field?

WDYT @rade @2opremio ?

@2opremio
Copy link
Contributor

2opremio commented Aug 10, 2016

WDYT @rade @2opremio ?

I think you are right and I started to completely remove the ID (namespace/resource_name) ... but then I realized we are also using them for the ranks. I could keep IDs internally just for the ranks but I think it would be fine to simply provide the namespace instead (nodes with the same namespace seem to be getting the same color anyways).

@davkal Can you check if this change would be acceptable?

@rade
Copy link
Member

rade commented Aug 10, 2016

Let's please not make #783 worse. See also #1567 (comment)

@davkal
Copy link
Contributor

davkal commented Aug 10, 2016

You can keep the ID, we simply dont need the field kubernetes_id in the nodes' metadata.

@2opremio
Copy link
Contributor

Let's please not make #783 worse

That's what I am asking, it seems we are only using the three first letters of the rank anyways.

You can keep the ID, we simply dont need the field kubernetes_id in the nodes' metadata.

I know I can keep it, but it would be nice to completely remove it if it doesn't make a difference (if it makes a difference then I can simply reconstruct the ID with the namespace and the name in the app without needing to move the ID value around). From a quick look at the JS code from @paulbellamy it seems that it doesn't but I would like to get a confirmation.

@rade
Copy link
Member

rade commented Aug 10, 2016

it seems we are only using the three first letters of the rank anyways.

Right now we are, but that must change.

@2opremio
Copy link
Contributor

Right now we are, but that must change.

Fair enough, then I will simply generate the exact same ranks in the app (concatenating the namespace and name), without requiring to pass the derivate ID from probe to app.

@davkal
Copy link
Contributor

davkal commented Aug 10, 2016

A change here should not affect rank values, whatever it's used for. This PR should only result in the kubernetes_id field not showing up for nodes. It's being read from a node's metadata object, so instead of filtering it from there in JS, I suggested of removing it from the metadata.

@rade
Copy link
Member

rade commented Aug 10, 2016

agreed. ditch kubernetes_id but leave ID well alone - if we want to make changes there let's do that in a separate issue/PR.

@2opremio
Copy link
Contributor

2opremio commented Aug 10, 2016

agreed. ditch kubernetes_id but leave ID well alone - if we want to make changes there let's do that in a separate issue/PR.

I already removed it, but the ID was only carried internally between probe and app (so it shouldn't impact the UI)

@2opremio 2opremio force-pushed the 1759-remove-k8s-id-col branch from b3f2b34 to 166bdf1 Compare August 10, 2016 16:01
@2opremio
Copy link
Contributor

I reverted the change in the UI and stopped reporting the ID from the probe.

@paulbellamy
Copy link
Contributor

LGTM

@davkal davkal assigned 2opremio and unassigned paulbellamy Aug 11, 2016
@2opremio 2opremio merged commit cfb686b into master Aug 11, 2016
@2opremio 2opremio deleted the 1759-remove-k8s-id-col branch August 11, 2016 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants