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.

Sign up for GitHub

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

Jump to bottom

Cluster node status #2341

Merged
merged 4 commits into from
Feb 22, 2024
Merged

Cluster node status #2341

merged 4 commits into from
Feb 22, 2024

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Feb 20, 2024 •
edited
Loading

Description

Parse cluster node status.

The precedence of values is:

  1. Offline
  2. Unclean
  3. StandBy
  4. Maintenance
  5. Shutdown/Pending/StandbyOnFail
  6. Online
  7. Unknown

In the fronted, (which will come later):

  • Online -> Bolt
  • Unclean/StandBy/Shutdown/Pending/StandByOnFail/Unknown -> Warning
  • Maintenance -> Maintenance tool
  • Offline -> Unplugged
    With a tooltip with the exact value

If you want, I can even add the FE part in this PR, but I thought to have it in other PR (even though I have the code).

How was this tested?

Tested

@arbulu89 arbulu89 added the enhancement New feature or request label Feb 20, 2024
@arbulu89 arbulu89 force-pushed the cluster-node-status branch from 0ea275d to a843199 Compare February 20, 2024 09:45
@arbulu89 arbulu89 marked this pull request as ready for review February 20, 2024 10:58
@arbulu89 arbulu89 force-pushed the cluster-node-status branch from 2627720 to c738226 Compare February 20, 2024 13:05
@arbulu89 arbulu89 mentioned this pull request Feb 20, 2024
Merged
rtorrero
rtorrero approved these changes Feb 20, 2024
View reviewed changes
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 @arbulu89 , I'd also address the frontend changes in a separate PR 👍

CDimonaco
CDimonaco reviewed Feb 21, 2024
View reviewed changes
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.

Everything looks good,

Could we have an enum for the status, in the domain and in the openapi schema?

@arbulu89
Copy link
Contributor Author

Everything looks good,

Could we have an enum for the status, in the domain and in the openapi schema?

I'm not really fond of using enums for values that are coming from the outside, that are not defined in trento itself.
They can change overtime, and this forces us to change the API version, as adding/removing values from the enum is a breaking change.

EMaksy
EMaksy approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

LGTM :)

@arbulu89 arbulu89 force-pushed the cluster-node-status branch from 135eb9c to 0b86c00 Compare February 22, 2024 10:38
@arbulu89 arbulu89 merged commit e401b3b into main Feb 22, 2024
24 checks passed
@arbulu89 arbulu89 deleted the cluster-node-status branch February 22, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CDimonaco CDimonaco CDimonaco left review comments

@EMaksy EMaksy EMaksy approved these changes

@rtorrero rtorrero rtorrero approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arbulu89 @rtorrero @CDimonaco @EMaksy