-
Notifications
You must be signed in to change notification settings - Fork 127
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
feat(k8s): wait for pool to be ready and add 'nodes' in its output #393
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.
Waiting on how the sdk PR goes to finish the review but a few things:
- waiting on the node ready on the read doesn't seems like a good idea (for instance if a node has crashed, and autohealing is off, the read will fail). Maybe just add it on create/upgrade
- add the name of the node :)
I added node name and pool ID in the output. |
I also moved the waiting for nodes in create/update only. Otherwise it leads to unpredictable behaviour as you mentioned. |
4e49d4e
to
dd12f2b
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.
It would be nice to add the wait_for_pool_ready
on the pool as well.
Also could you add some tests to check that the behaviour is ok:
- just wait on the pools ready, and check that the status of the nodes is running and the number is correct
- also modify the documentation to reflect the changes
0606f8f
to
49c55a4
Compare
fb3593d
to
c9ee2a6
Compare
LGTM |
6419b53
to
b2b95d5
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
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 @Sh4d1 @loicbourgois @kindermoumoute ! |
Depends on scaleway/scaleway-sdk-go#312To test with the scaleway-sdk-go patch from scaleway/scaleway-sdk-go#312,run:
=> scaleway/scaleway-sdk-go#312 was merged