-
Notifications
You must be signed in to change notification settings - Fork 4
Adds support for testing on non-localhost docker #47
Conversation
|
I'm not entirely sure why I didn't need to change https://github.com/MITLibraries/oastats-backend/blob/master/pipeline/request_writer.py#L26 The tests pass and I'm not entirely sure how that works so I'll point it out to you for consideration before we merge. 🔮 |
tests/run_integration.sh
Outdated
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.
Let's change this line to:
if docker-machine -v 2>/dev/null; then
The brackets aren't needed for a command test and we should suppress error messages in this case.
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.
Thanks. I suck at bash. Will update.
|
The tests pass because the mongo uri in 953ac8c#diff-00cd37d3fc324762195eb40f56bb1102R26 is passed as a list with one item. This works, but I think I would be more comfortable if we just change things to only ever work with a mongo uri. As it stands now, if the mongo uri is not wrapped in a list the connection will fail. With the one change noted above I'd say go ahead and merge. I'll create an issue to change the mongo connection. |
|
I'm running into some local issues with the docker-machine logic. I believe docker-machine is as buggy as the rest of the docker tools. I'll leave this open for a bit in the hopes I come up with something stable. |
953ac8c to
eed7096
Compare
|
@gravesm This seems better to me as it tries to be less clever and thus is much more reliable. I'll squash out the extra commit once you have a chance to look at the changes. Before merge, it would probably be best if you checked this out and ran the tests locally to ensure it actually works as intended on linux since the Travis tests use different logic to kick them off. |
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.
It turns out that most of my problems were just not realizing we had to white list necessary env in tox.ini even though the bash script runs with #!/usr/bin/env bash
|
With the few comments, 👍 Nice work figuring this out. |
|
Thanks for the review. I'll make those changes and squash this up. |
bcd88cc to
d80b783
Compare
Adds support for testing on non-localhost docker
Supports docker not running on localhost. Or at least the subset of
that possibility in which
docker-machineis being used.closes #44