-
Notifications
You must be signed in to change notification settings - Fork 15
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
Saptune summary warning icon #2187
Conversation
ba83ca9
to
39795b5
Compare
39795b5
to
685117d
Compare
685117d
to
a12c3b4
Compare
a12c3b4
to
d468236
Compare
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.
Hey @EMaksy ,
Did you discuss the scenario where nodes without saptune will display this as well with @abravosuse ?
I mean, in that scenario, the host health is not changed to warning, and I think, this story comes from the need to reflect the warning point in the details view when the host list shows a warning health
@EMaksy just to clarify the discussion we had on Friday: Right now, when a version of saptune lower than 3.1.0 is detected in a host, a warning icon is displayed in the corresponding details view, regardless whether there is an SAP workload or not. But only in the case there is such workload, the warning gets added to the aggregated health of the host. The behavior should be the same when no saptune is detected:
|
Thanks guys, for clarifying, I will ensure that it behaves like @abravosuse describes it 👍 |
@abravosuse @EMaksy if this is the case, i think the code might be ok. |
d468236
to
6b8ac94
Compare
@EMaksy apologies for the back and forth. Xabi has a point. Please scratch what I said before and go for the following behavior:
|
aaf4629
to
eeec4ca
Compare
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.
Good!
I think it makes the job
if (!version) { | ||
return <span>Not installed</span>; | ||
return isSapPresent ? ( |
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.
Not the most beautiful piece of code, but I guess it works
36336dd
to
bbd716f
Compare
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 for me,
As a suggestion, I reccomend to change the name of props from isSapPresent
to sapPresent
.
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 thanks @EMaksy
bd79dc4
to
90799fb
Compare
Description
Add a warning state icon to the saptune overview when no saptune version is provided and sap workload is happening on the host
Left on host with sap workload and right without.

Preview:
How was this tested?
Updated existing good test to check if the icon was rendered.