-
Notifications
You must be signed in to change notification settings - Fork 14
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
Cluster maintenance mode #2297
Cluster maintenance mode #2297
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.
Just a tiny change request but overall LGTM
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 tried to submit the change request but it didn't persist 😄
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.
Looks good.
Some suggestions from my side.
Anyway, when I say maintenance state in the frontend instead of maintenance mode my brain almost exploded XD
Fix the factory at least, the tests are passing my merge coincidence, because you get false by default with lodash in the code
Thx for spotting all the stuff, everything is fixed, re-request review! |
986fa14
to
7b888ee
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.
Thank you @CDimonaco
All good from my side.
I did some tiny comments, but feel free to merge without a new review
@@ -67,6 +67,7 @@ export const hanaClusterDetailsFactory = Factory.define(() => { | |||
stopped_resources: clusterResourceFactory.buildList(2), | |||
system_replication_mode: 'sync', | |||
system_replication_operation_mode: 'logreplay', | |||
maintenance_mode: false, |
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.
By the way, we should include this maintenance_mode
entry in the ascsErsClusterDetailsFactory
as well
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.
done
@@ -74,11 +74,11 @@ context('HANA cluster details', () => { | |||
.contains(availableHanaCluster.hanaSecondarySyncState); | |||
}); | |||
|
|||
it(`should have sap hana sr health state ${availableHanaCluster.sapHanaSRHealthState}`, () => { | |||
it(`should have maintenance mode ${availableHanaCluster.maintenanceState}`, () => { |
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 still see maintenanceState
here 🙈
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.
done
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.
🔥
7b888ee
to
0617d37
Compare
Everything done! Thx for the feedbacks, merging when ci is green |
Description
Cluster maintenance mode.
Available for scale up, scale out and ascs/ers
We get the information through the
cibadmin
command.How was this tested?
Automated tests, backend, frontend and e2e