Support displaying multiple ips #15743
Conversation
mantis-toboggan-md
left a comment
There was a problem hiding this comment.
The InternalExternalP formatter is also used in the cluster detail page; this PR has broken it such that it always shows - / Same as External
I think what needs to happen is for the updated getters to be added to cluster.x-k8s.io.machine model to support rke2/k3s clusters detail page. At a glace it looks like they'd be veeery similar to what you have in the node model already. I think the management.cattle.io.node model is only used for rke1 detail page so we should be ok to leave it alone, but worth double-checking that
thanks for catching this, since the formatter was updated to receive an array I've updated |
| return this.internalIps[0]; | ||
| } | ||
|
|
||
| return this.t('generic.none'); |
There was a problem hiding this comment.
I intentionally didn't add back returning the translation string for consistency and also the risk of checking for !!externalIp, the formatter will take care of absent ips by returning "-".
There was a problem hiding this comment.
That makes sense to me. I checked usage of internalIp and externalIp in the dashboard repo looking for possible regressions and I think this change is safe.
mantis-toboggan-md
left a comment
There was a problem hiding this comment.
I had one minor issue, PR otherwise looks good
| / | ||
| <template v-if="internalSameAsExternal && isIp(row.internalIp)"> | ||
| <span class="separator">/</span> | ||
| <template v-if="internalSameAsExternal"> |
There was a problem hiding this comment.
This formatter will render - / Same as External when neither internal nor external IPs are defined. Though technically they are the same when they're both null, I think we still want the table formatter to render - / - in that case. I think one way of doing that would be changing this line to something like
<template v-if="internalSameAsExternal && internalIp">
There was a problem hiding this comment.
makes sense, I updated the logic inside the computed property internalSameAsExternal
| return this.internalIps[0]; | ||
| } | ||
|
|
||
| return this.t('generic.none'); |
There was a problem hiding this comment.
That makes sense to me. I checked usage of internalIp and externalIp in the dashboard repo looking for possible regressions and I think this change is safe.
* support displaying multiple ips in the table * fix not passing an array to ips formatter from model * minor styling * improve logic for internalSameAsExternal --------- Co-authored-by: Mo Mesgin <[email protected]>
Summary
Fixes #15588
Occurred changes and/or fixed issues
Please check the UI requirements added to the issue.
Technical notes summary
InternalExternalIP.vuenode.jsandcluster.x-k8s.io.machinemodels, they now return array of ips to the formatter and the formatter takes care of displaying multiple ips. Also for the single IPs, the first item in the array will be returned likekubectlrather than the last one.Areas or cases that should be tested
Go to Explore > Cluster > Nodes:
Areas which could experience regressions
Check the IP columns in the tables on these pages for both single and multiple IPs:
Check the Target column on(finding a case with an external IP would be great):
Screenshot/Video
pripv.mov
Checklist
Admin,Standard UserandUser Base