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

Saptune summary warning icon #2187

Merged
merged 7 commits into from
Jan 24, 2024
Merged

Saptune summary warning icon #2187

merged 7 commits into from
Jan 24, 2024

Conversation

EMaksy
Copy link
Member

@EMaksy EMaksy commented Jan 17, 2024

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:
image

How was this tested?

Updated existing good test to check if the icon was rendered.

@EMaksy EMaksy added the javascript Pull requests that update Javascript code label Jan 17, 2024
@EMaksy EMaksy self-assigned this Jan 17, 2024
@EMaksy EMaksy added the env Create an ephimeral environment for the pr branch label Jan 17, 2024
@EMaksy EMaksy force-pushed the saptune_summary_warning_icon branch from ba83ca9 to 39795b5 Compare January 18, 2024 09:34
@EMaksy EMaksy added env Create an ephimeral environment for the pr branch and removed env Create an ephimeral environment for the pr branch labels Jan 18, 2024
@EMaksy EMaksy force-pushed the saptune_summary_warning_icon branch from 39795b5 to 685117d Compare January 18, 2024 14:11
@EMaksy EMaksy removed the env Create an ephimeral environment for the pr branch label Jan 19, 2024
@EMaksy EMaksy force-pushed the saptune_summary_warning_icon branch from 685117d to a12c3b4 Compare January 19, 2024 09:35
@EMaksy EMaksy marked this pull request as ready for review January 19, 2024 09:36
@EMaksy EMaksy force-pushed the saptune_summary_warning_icon branch 2 times, most recently from a12c3b4 to d468236 Compare January 19, 2024 14:07
Copy link
Contributor

@arbulu89 arbulu89 left a 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

@abravosuse
Copy link
Contributor

@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:

  • 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

@EMaksy
Copy link
Member Author

EMaksy commented Jan 22, 2024

Thanks guys, for clarifying, I will ensure that it behaves like @abravosuse describes it 👍

@arbulu89
Copy link
Contributor

@abravosuse @EMaksy if this is the case, i think the code might be ok.
Anyway, i'm surprised of this desired behaviour. Showing a warning icon if saptune is not installed in a host that doesn't need saptune doesn't look really appropriate for me. Specially because the host health is not affected by it.
My 2 cents.

@EMaksy EMaksy force-pushed the saptune_summary_warning_icon branch from d468236 to 6b8ac94 Compare January 22, 2024 10:25
@abravosuse
Copy link
Contributor

@EMaksy apologies for the back and forth. Xabi has a point. Please scratch what I said before and go for the following behavior:

  • a warning icon is displayed in the corresponding details view only if there is an SAP workload in the host
  • in that case, the warning is added to the aggregated health of the host (this last part is already implemented!)

@EMaksy EMaksy force-pushed the saptune_summary_warning_icon branch 2 times, most recently from aaf4629 to eeec4ca Compare January 23, 2024 13:47
Copy link
Contributor

@arbulu89 arbulu89 left a 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 ? (
Copy link
Contributor

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

@EMaksy EMaksy force-pushed the saptune_summary_warning_icon branch from 36336dd to bbd716f Compare January 24, 2024 10:04
Copy link
Member

@CDimonaco CDimonaco left a 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.

Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @EMaksy

@EMaksy EMaksy force-pushed the saptune_summary_warning_icon branch from bd79dc4 to 90799fb Compare January 24, 2024 12:45
@EMaksy EMaksy merged commit 1e84440 into main Jan 24, 2024
24 checks passed
@EMaksy EMaksy deleted the saptune_summary_warning_icon branch January 24, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code
Development

Successfully merging this pull request may close these issues.

None yet

5 participants