-
Notifications
You must be signed in to change notification settings - Fork 10
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Hi @sicoyle |
Hey @sicoyle According to the code in the What I understand from a brief look at the code (I might be wrong) is that if the service is not available after Another thing: ./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. As opposed to the binary generated from the 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 If you can fix this issue too in this PR, then it'd be great. |
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.
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!
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 await
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: