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

feat(wait): define wait pkg and changes to enhance pkg experience #5

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sicoyle
Copy link

@sicoyle sicoyle commented Dec 13, 2022

The current implementation removes the ability for others to consume the wait logic as a Go package. For my current project, we wanted our Go microservices running natively to have the same service dependency waiting logic as when we ran our services within a Docker environment. This need had us copy/paste (with attributions of course :) ) the wait logic defined in your repository within our pkg/ section of our project to use within our Go code instead of within the Docker layer of our microservices. However, we would like to enhance this implementation to leverage Go packages such that other gophers may consume this logic as well moving forward. Therefore, this PR brings about a wait package, wait.ForDependencies(...) addition, and minor modifications along the way.

For our project, we wanted to have a max timeout set for our services such that it could have the ability to retry checking its dependencies. I could see this as an integer retry attempt count in the future instead, or, in addition to this implementation. I could also see moving forward the potential for there to be a setup along the lines of:
wait.ForDependencies(...).WithRetryAttempts(<insert #>)
AND/OR
wait.ForDependencies(...).WithRetryTimeout(<insert time.Duration timeout>)

I figured with this PR I would keep it simple with what we implemented on our end to get the Go package out there for others to use if they would like, and there could be discussions going forward on enhancing it again with the aforementioned future enhancements.

I also tried to ensure that the Go utility and package had similar logic in terms of service request timeouts and max service timeout for retrying request. This topic came about during a team discussion we had on "what are our expectations if dependencies never become available. Copy/pasting in your wait logic made us realize that services would hang forever because that timeout logic was on the utility side of things...

All of that is to say I look forward to your feedback and hope you like some of these changes!

With the changes in this PR, you can expect the following output after copy/pasting in some of your sample commands from the readme:

$ ./wait-for-it -w google.com:80 -w localhost:27017 -t 30 -- echo "Waiting for 30 seconds for google.com:80 and localhost:27017"
wait-for-it: waiting 30 seconds for google.com:80, localhost:27017 for a max of 30 seconds
wait-for-it: failed to dial service localhost:27017 with err: dial tcp 127.0.0.1:27017: connect: connection refused
wait-for-it: google.com:80 is available after 70.906518ms
Waiting for 30 seconds for google.com:80 and localhost:27017

$ ./wait-for-it -w abcd:80 -s -t 5 -m 10 -- echo "Done\!"
wait-for-it: waiting 5 seconds for abcd:80 for a max of 10 seconds
wait-for-it: failed to dial service abcd:80 with err: dial tcp: lookup abcd on XXX.XX.XXX.X:XX: no such host
wait-for-it: strict mode, refusing to execute subprocess

$ ./wait-for-it -w google.com:80 -- echo "it works"
wait-for-it: waiting 15 seconds for google.com:80 for a max of 30 seconds
wait-for-it: google.com:80 is available after 39.666269ms
it works

@sicoyle
Copy link
Author

sicoyle commented Dec 13, 2022

@roerohan

@roerohan
Copy link
Owner

Hi @sicoyle
Sorry, it's been a while since I last checked out this package, hence the delayed response.
The PR looks good, I'll just test it out once, and merge your PR if it works properly.
Thanks for the contribution 🥳

@roerohan
Copy link
Owner

Hey @sicoyle
Could you please elaborate on how max timeout is different from timeout?

According to the code in the main branch, the timeout indicates the maximum amount of time the tool waits for a response from the specified service, as is also the case in the bash package for wait-for-it.

What I understand from a brief look at the code (I might be wrong) is that if the service is not available after timeout seconds, it will wait for another few seconds until a total of max timeout seconds. If this is what is intended, I still do not get the point of the original timeout, why can we not just have one timeout in favor of having a simpler API?

Another thing:
When I use the binary created from your branch and run this command

./wait-for-it -s -w google.com:80 -w localhost:27017 -t 15 -m 30 -- echo "Waiting for 30 seconds for google.com:80 and localhost:27017"

It fails right away and does not wait for any amount of time.
image

As opposed to the binary generated from the main branch, where it would wait for 15 seconds.

go build -o ./wait-for-it-main # on the main branch
./wait-for-it-main -s -w google.com:80 -w localhost:27017 -t 15 -- echo "Waiting for 30 seconds for google.com:80 and localhost:27017"

Here are some supporting screenshots for the wait-for-it-main binary.

  • When the command was run
    image

  • After 15 seconds
    image

If you can fix this issue too in this PR, then it'd be great.
Also, I love how the code is structured and written, thanks for making it clean and easy-to-read.

Copy link
Owner

@roerohan roerohan left a comment

Choose a reason for hiding this comment

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

Since the package will break if I merge this according to my previous comment, I'm making this as Changes Requested. I'll update it when these changes are in place. Thanks again for your interest in this repository!

@roerohan roerohan added the enhancement New feature or request label Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants