Open johncowen opened 2 weeks ago
Name | Link |
---|---|
Latest commit | c42fe8de8a10144cd1104567d394162eecc6314f |
Latest deploy log | https://app.netlify.com/sites/kuma-gui/deploys/67445dfc01bad700085bb051 |
Deploy Preview | https://deploy-preview-3186--kuma-gui.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Looks great! There's only 1 bug with clusters
https://konghq.zoom.us/clips/share/A2F3MRZnZDRPSjZfT1E5bVhNRTRERWl1MnlnAQ
For the inbound issue I observe: https://github.com/kumahq/kuma/issues/12123
PR to fix up the issue with clusters is here:
note: I removed an annoying ESLint rule here https://eslint.org/docs/latest/rules/multiline-ternary#when-not-to-use-it . We don't have any conventions in regards to ternaries, do whatever works for you. Generally you've decided to use a ternary because the format you've personally chosen reads nicer - furthermore its pointless having a eslint rule to force wrapping of something that is designed for single lines. Not only that but the eslint rule we had makes these super hard to read.
I agree with this π since you speak about readability, I'd like to bring up no-nested-ternary π I think this might be a good addition to our lint rules as it enforces short and one-lined ternary expressions, which are great. But when it comes to more complex flows, I'd rather use if-else
or a mix of ternary and if-else
. Nesting ternaries hurts readability IMO. (But have not seen any nested ones yet tbh π )
I agree with this π since you speak about readability, I'd like to bring up no-nested-ternary π
More than happy to take look at it.
The only thing is ternaries are about more than readability, they allow you to have type safety at runtime in javascript, which while I don't think is as important as the type safety, I believe is also a micro-performance improvement in javascript engines (but lets not get into micro-optimizations, thats side benefit)
Its not so much using the ternaries, its more the avoidance of using let
.
let a = '1'
if(true) {
a = '2'
}
// a is a string
vs
const a = true ? '2' : '1'
// a is '1' | '2'
In the second form:
a
can only ever be '1'
or '2'
, not any-string (in TS) or anything (in JS), and therefore optimize.a
will never be set again, and therefore optimize.So I use these inline forms a lot (and did previous to Typescript) via ternaries and inline functions for try/catch
and switch
among other constructs for these reasons, not necessarily for readability. If there's a nested ternary somewhere there's a chance its for the above reasons.
I have an example of a change a made recently that outlines this, lemme try and find it.
edit:
Not a ternary but its the same idea, if you take a look at this code from a recent PR (π oh its actually this PR):
In https://github.com/kumahq/kuma-gui/pull/3186 I'm changing it to this:
Using the inline function gives me automatic type safety both in TS at build time and JS at runtime, and honestly in this case, in avoiding the multiple if conditionals the inline-function/switch is far easier to read to my eyes at least.
I agree with this π since you speak about readability, I'd like to bring up no-nested-ternary π
More than happy to take look at it.
The only thing is ternaries are about more than readability, they allow you to have type safety at runtime in javascript, which while I don't think is as important as the type safety, I believe is also a micro-performance improvement in javascript engines (but lets not get into micro-optimizations, thats side benefit)
Its not so much using the ternaries, its more the avoidance of using
let
.let a = '1' if(true) { a = '2' } // a is a string
vs
const a = true ? '2' : '1' // a is '1' | '2'
In the second form:
- The JS engine knows
a
can only ever be'1'
or'2'
, not any-string (in TS) or anything (in JS), and therefore optimize.- The JS engine knows that
a
will never be set again, and therefore optimize.So I use these inline forms a lot (and did previous to Typescript) via ternaries and inline functions for
try/catch
andswitch
among other constructs for these reasons, not necessarily for readability. If there's a nested ternary somewhere there's a chance its for the above reasons.I have an example of a change a made recently that outlines this, lemme try and find it.
edit:
Not a ternary but its the same idea, if you take a look at this code from a recent PR (π oh its actually this PR):
In #3186 I'm changing it to this:
Using the inline function gives me automatic type safety both in TS at build time and JS at runtime, and honestly in this case, in avoiding the multiple if conditionals the inline-function/switch is far easier to read to my eyes at least.
Yeah, I like the switch-case
π π another great way of avoiding nested (or chained) ternaries, having the optimization benefits and it's a lot easier to read and understand.
Adds XDS configuration drawers to inbounds and outbounds so show only the listeners/clusters relevant to that inbound/outbound.
Closes https://github.com/kumahq/kuma-gui/issues/3182 Closes https://github.com/kumahq/kuma-gui/issues/3181
You can run this against a local cluster by setting a cookie like the following:
note: I removed an annoying ESLint rule here https://eslint.org/docs/latest/rules/multiline-ternary#when-not-to-use-it . We don't have any conventions in regards to ternaries, do whatever works for you. Generally you've decided to use a ternary because the format you've personally chosen reads nicer - furthermore its pointless having a eslint rule to force wrapping of something that is designed for single lines. Not only that but the eslint rule we had makes these super hard to read.
~Note: This will not pass lint tests and is purposefully an early WIP PRed for preview site purposes.~
Notes taken during work:
Adding some pseudo code to help explain the filtering/selecting we are doing:
Inbounds
I use
'127.0.0.1:0000'
below in place of the actualdataplane.networking.address
dynamic_listeners[].name === '127.0.0.1:0000'
{dynamic_active_clusters, static_clusters}.cluster.load_assignment.endpoints[].lb_endpoints[].endpoint.address.socket_address.{address:port_value} === '127.0.0.1:0000'
(I'm not totally sure we should be using this one)Note for an inbound we do not know the type of the service (i.e. is it a
_msvc_
a_mzsvc_
etc) so we can't filter by cluster name.Improvements: we should look for
inbound:ignore.ip.address:0000
use prefix/suffix because we need to be careful with ipv6.dynamic_listeners[].name === 'inbound:ignore.ip.address:0000'
dynamic_active_clusters[].cluster.name === 'ignore.host:0000'
Outbounds
I use
outbound
below in place of the actual Envoy stats name (e.g.default_demo-app_kuma-demo_default_msvc_5000
)dynamic_active_clusters[].cluster.name === <outbound>
dynamic_endpoint_configs[].endpoint_config.cluster_name === <outbound>
Note we don't have any information about a specific outbound apart from the clusterName/envoy stats prefix. We can take information from the dataplane associated with the outbound, but not the outbound itself.
Improvements:
dynamic_listeners[].name === 'outbound:<outbound>'
to be future proof with https://github.com/kumahq/kuma/issues/12093.If we need to add more instance of filtering/selecting it would be helpful to express what needs to be done using the above examples