-
Notifications
You must be signed in to change notification settings - Fork 55
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
Test with Zipkin Server #150
Conversation
PTAL at the approach for zipkin server test. The actual call to start this test will be moved to run iff on mater branch |
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.
In general, I like this approach as it is similar to other ways we test and integrate. We should add a comment somewhere explaining why we need to unzip, or a TODO to determine why.
I am a little uncomfortable about copy/pasting a wait-for-it script. If wget, curl or otherwise don't support a retry loop, I'd prefer a simple shell loop like what we use in cassandra vs a copy/paste of ambiguously licensed stuff
timeout=300
while [[ "$timeout" -gt 0 ]] && ! /cassandra/bin/cqlsh -e 'SHOW VERSION' localhost >/dev/null 2>/dev/null; do
echo "Waiting ${timeout} seconds for cassandra to come up"
sleep 10
timeout=$(($timeout - 10))
done
https://github.com/openzipkin/docker-zipkin/blob/master/cassandra/install.sh#L33-L38
cc @abesto
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.
I am a little uncomfortable about copy/pasting a wait-for-it script.
My 2c on https://github.com/vishnubob/wait-for-it: this looks like a centralized, "standard" solution for a common problem. In principle I'm all for using it instead of rolling our own. Licensing on it is quite clear: it's MIT, meaning we must include the license notice along with the script, maybe in a header. Apparently it's packaged for both Debian and Ubuntu, so maybe apt-get install
-ing it would be cleaner?
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.
Only nits from my side.
Good call. Will update. |
oops - not in ubuntu xenial :( |
good news - i think i got the script to run the integration test pretty well. https://travis-ci.org/openzipkin/zipkin-gcp/builds/585941140#L4988
|
@saturnism I raised a PR #154 to fix the classpath thing |
359f41f
to
63a8fbf
Compare
ok pass.. now I'm going to un-vendor wait-for-it using a stable copy we control. https://raw.githubusercontent.com/openzipkin-contrib/wait-for-it/master/wait-for-it.sh |
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.
If this fails master, it will help those doing releases know there's a problem. For example, it is intuitive to look at Travis prior to cutting a release.
I think it is important to do this step regardless of Docker as not everyone looks at Docker, and also we'll know if there's an issue earlier.
Cool stuff @saturnism. Just verified master, and I think we can finally cut a release safely :) Thanks for all the support on this @anuraaga @elefeint @abesto
|
#130