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

Add client.wait method #15

Merged
merged 1 commit into from
Nov 19, 2021
Merged

Conversation

knkski
Copy link
Contributor

@knkski knkski commented Nov 3, 2021

Adds client.wait as a rough analogue of kubectl wait. Builds on top of client.watch to wait for specified resource to enter expected state.

Opening as draft PR to ensure that this is something that is generally desired in lightkube. If so, will also implement logic for AsyncClient and convert to regular PR.

Punts on implementing an equivalent of kubectl wait's --timeout flag, as that would require some deeper changes in how exactly the watch requests are handled. Some extra logic around stream=True and setting the read timeout should work if that's desired, but this PR also includes an example of how to achieve the same thing with gevent.Timeout.

@gtsystem
Copy link
Owner

gtsystem commented Nov 4, 2021

I think adding this functionality is a nice idea, as it's not straight forward to implement.

Few things I was thinking about regarding the interface:

  • It would be nicer to align the naming more with kubectl, so instead of success_conditions maybe for_conditions.
  • In the future we could also add support for "wait for delete" using for_delete=True.
  • Failure cases are not supported by kubectl but I think it's a good addition, we could name it raise_for_conditions (this clarify that the effect is an exception and is similar to requests method raise_for_status ).
  • Defaults: I'm not sure providing this defaults are a good idea:
    1. Success conditions can vary across different resources and new ones can come in the future, once we define this default DEFAULT_SUCCESS_CONDITIONS we cannot modify it as it would be a breaking change. I would prefer this parameter to be required instead (as in kubectl wait)
    2. Checking for failures may not be always wanted and to disable this default the user will be force to add raise_for_status=[] which is not ideal. Empty should be the default instead.
  • kubectl supports passing the condition status to verify (true or false). Not sure how useful this is but we could support this in the future using a dict like value instead for_conditions={"Available": False}.
  • timeout: This is something we may want to support but we could go without it for the first version.

Any opinion/idea?

lightkube/core/client.py Outdated Show resolved Hide resolved
lightkube/core/client.py Outdated Show resolved Hide resolved
@knkski knkski marked this pull request as ready for review November 12, 2021 20:15
@knkski
Copy link
Contributor Author

knkski commented Nov 12, 2021

Thanks for the review, and sorry for the slow response. I've updated this branch with all requested changes other than the timeouts and for_conditions supporting a dict-like value. I've also added AsyncClient.wait to mirror Client.wait.

Copy link
Owner

@gtsystem gtsystem left a comment

Choose a reason for hiding this comment

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

Code coverage for the two new functions is missing. Tests should go in tests/test_client.py and tests/test_async_client.py. Be sure we are covering every condition.

lightkube/core/client.py Outdated Show resolved Hide resolved
lightkube/core/client.py Outdated Show resolved Hide resolved
lightkube/core/client.py Outdated Show resolved Hide resolved
lightkube/core/client.py Outdated Show resolved Hide resolved
lightkube/core/client.py Outdated Show resolved Hide resolved
e2e-tests/test_client.py Outdated Show resolved Hide resolved
@knkski knkski force-pushed the add-client-wait branch 3 times, most recently from 01ad53c to d23677b Compare November 15, 2021 17:14
e2e-tests/test_client.py Outdated Show resolved Hide resolved
@knkski knkski force-pushed the add-client-wait branch 2 times, most recently from cf3df80 to b235719 Compare November 18, 2021 19:13
Adds client.wait as a rough analogue of kubectl wait. Builds on top of
client.watch to wait for specified resource to enter expected state.
@gtsystem gtsystem merged commit 6ead794 into gtsystem:master Nov 19, 2021
@gtsystem
Copy link
Owner

Merged. @knkski, thanks a lot for the contribution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants