-
Notifications
You must be signed in to change notification settings - Fork 69
wait_for_ingress_endpoint #779
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
wait_for_ingress_endpoint #779
Conversation
src/warnet/dashboard.py
Outdated
| timeout = 300 | ||
| click.echo(f"Waiting {timeout} seconds for ingress endpoint ...") | ||
| wait_for_ingress_endpoint(timeout=timeout) |
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.
Mhh, maybe this hint to the user that we’re waiting for something isn’t needed, since that wasn’t the case before either.
068fa3d to
3491dff
Compare
|
comparing against main branch with NO network deployed, the current error is much cleaner: mainPR |
2e5b7d3 to
ad3e6b7
Compare
d5d0256 to
47aeee2
Compare
|
Ohh, right! I wrapped When there's no network deployed, it now prints this: (The previous hint was misleading; |
pinheadmz
left a comment
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.
Tested with docker and minikube, tried to break it, looks good all around. One comment below to be specific because even though a network is running does not garuntee it will ever have an ingress. fix that and we'll merge
Co-authored-by: Matthew Zipkin <pinheadmz@pm.me>
4b27c3c to
afe8321
Compare
This PR introduces
wait_for_ingress_endpoint, the helper function mentioned in #766 (comment).It checks every second if ingress is up, in the same way
get_ingress_ip_or_hosttries to return the ingress IP address or hostname. So whenwait_for_ingress_endpointdid not throw,get_ingress_ip_or_hostshould always succeed.I considered calling
wait_for_ingress_controllerinside it, but I think it would be a redundant check.After this PR has been reviewed, approved, and a new version of warnet has been released, I will create PRs in battle-of-galen-erso and wrath-of-nalo to use
wait_for_ingress_endpointinstead ofwait_for_ingress_controllerin their dashboard upload scripts. This should then fix what #766 intended to fix.I tested this by deploying
regtest_jamfrom wrath-of-nalo and then runningpython -c "from warnet.k8s import wait_for_ingress_endpoint; wait_for_ingress_endpoint(timeout=5)". I got the timeout error withoutminikube tunnelrunning and no timeout error with it running.Since the return value of
get_ingress_ip_or_hostis now no longer used to print the hint for the user, we can now consider making it throw instead of returningNone(failing fast instead of potentially obscure error later). However, it's also used in the tests, so they would break without an update. I didn't want to make a decision on this and mess with the tests in this PR to keep the scope low.