Add IstioConfig under Workload Details #1901
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hhovsepy I've fixed the icon problem. But the column is more tricky due the change should be in the PF4 component, so for that, I've re-arranged the columns, to put a 10% on the first of all of them, otherwise columns would like different. So, with the change we'll have: I've tested several options and no one is 100% good, so, at the moment you put a different % in a middle component, columns tend to have different aspect. So, in short widths: So, let me know what do you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified the warning icon and the layout. Much better now @lucasponce thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! There is one comment I'd like to hear from @lucasponce
let istioConfigIcon = undefined; | ||
if (this.state.workloadIstioConfig?.validations) { | ||
const typeNames: { [key: string]: string[] } = {}; | ||
if (this.state.workloadIstioConfig.validations['gateway']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably style, but do you think it is a way to do this if/else statements shorter not repeating code?
I guess that using dynamic access to objects of JS/TS could help reduce the number of lines of code.
this.state.workloadIstioConfig["envoyFilters"].forEach()
could work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good comment, on this side I think this would work.
if (this.state.workloadIstioConfig?.validations) { | ||
const typeNames: { [key: string]: string[] } = {}; | ||
if (this.state.workloadIstioConfig.validations['gateway']) { | ||
this.state.workloadIstioConfig.gateways?.forEach(gw => typeNames['gateway'].push(gw.metadata.name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the other if statements, you set the typeNames
to an empty array. The lack of it for the gateways is intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's filtering in the UI side the validations that belongs to the Istio Config resources that are linked with a Workload.
So an empty names array is part of the logic.
: object && !object.valid && warnChecks > 0 | ||
? ValidationTypes.Warning | ||
: ValidationTypes.Correct; | ||
return errChecks > 0 ? ValidationTypes.Error : warnChecks > 0 ? ValidationTypes.Warning : ValidationTypes.Correct; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it simplifies the logic. Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Related to kiali/kiali#3015
Requires kiali/kiali#3157
It adds IstioConfig linked with a workload (resources with some workloadSelector that matches the workload labels).
For testers, I've used this (somehow artificial) example (you can modify it inside Kiali to check that validations are working):