Skip to content
This repository has been archived by the owner on Dec 5, 2023. It is now read-only.

Make rabbitmq host in queue-master configurable just like in shipping #17

Merged
merged 6 commits into from
Apr 11, 2017

Conversation

rberlind
Copy link
Contributor

I'm submitting this pull request in connection with issue microservices-demo/microservices-demo#686 and #16 which contains excerpts from the first.

The code changes are copied directly from shipping, so should not cause any problems.

I also just successfully ran ./test/test.sh unit.py and ./test/test.sh component.py against the modified code in my cloned repository and both were successful, although the latter did give exception "org.springframework.amqp.AmqpIOException: java.net.UnknownHostException: rabbitmq" because there was no rabbitmq host on which rabbitmq could be listening. But I think that's ok.

@rberlind
Copy link
Contributor Author

The coveralls.py check failed, but I believe that is because the COVERALLS_TOKEN string was empty for me and that this might always occur for forks since my repository does not have the actual token that you guys have in the main microservices-demo repository. I did just log into travis-ci.org with my GitHub password. Perhaps that will enable the COVERALLS_TOKEN to be set correctly in the future?

Is there a way to re-run the test?

@rberlind rberlind closed this Mar 28, 2017
@rberlind
Copy link
Contributor Author

I'm going to close and resubmit this in the hope that the Travis tests will run correctly since I have now linked my Travis CI account to my GitHub account.

@rberlind rberlind reopened this Mar 28, 2017
@rberlind
Copy link
Contributor Author

I also logged into coveralls.io and enabled this repository there. I will close and re-open and see if that successfully builds.

@rberlind rberlind closed this Mar 28, 2017
@rberlind
Copy link
Contributor Author

Re-opening to rerun test

@rberlind rberlind reopened this Mar 28, 2017
@rberlind
Copy link
Contributor Author

No matter what I do, it seems that the COVERALLS_TOKEN string is blank, causing the coveralls.py test to fail. This might be typical with forked repositories according to https://forum.micropython.org/viewtopic.php?t=3007. But maybe I just haven't set things up the right way so that COVERALLS_TOKEN is populated correctly when I submit a pull request here. Please advise.

@rberlind
Copy link
Contributor Author

rberlind commented Apr 1, 2017

Any chance of getting this committed early next week? This is the last thing I need to enable running the entire Socks Shop app on Apcera with docker images instead of having to download and modify source code from GitHub. We would like to show this at DockerCon in a few weeks.

Thanks,
Roger Berlind
Apcera

@rberlind rberlind closed this Apr 3, 2017
@rberlind
Copy link
Contributor Author

rberlind commented Apr 3, 2017

Re-opening after turning switch on for my repository on travis-ci.org to see if the coveralls test will pass . I suspect not since I probably still won't have access to your coveralls token.

@rberlind rberlind reopened this Apr 3, 2017
@rberlind
Copy link
Contributor Author

rberlind commented Apr 3, 2017

This time, closing and re-opening did not even cause Travis to run the test again.

@rberlind
Copy link
Contributor Author

rberlind commented Apr 3, 2017

I pushed another commit to comment out the spawning of Docker, in line with #3. Hoping that closing and re-opening will pull in that commit.

@rberlind rberlind closed this Apr 3, 2017
@rberlind rberlind reopened this Apr 3, 2017
@rberlind
Copy link
Contributor Author

rberlind commented Apr 3, 2017

Commenting out docker.init() and docker.spawn() lines is ok, but commenting out DockerSpawner was not since that leaves the ShippingTaskHandler without any bean that qualifies as autowire candidate for the Shipment dependency. Now the test fails in coveralls test again. Note that the prior test cannot connect to rabbitmq server, but none is spun up during the test. So, that is presumably expected.

If the reviewers object to my commenting out the docker.init() and spawn() lines, I can do another commit and restore them.

@lukemarsden
Copy link

Hi @rberlind, thanks for your PR! I'm sorry but we won't have time to review it before DockerCon next week, however please let me know if I can help you building custom images so that you can demo from your fork.

@rberlind
Copy link
Contributor Author

Thanks Luke,
If you don't have time, it's not a big deal. The fact that queue-master can't connect to rabbitmq is not really a big deal since it doesn't affect the UI. Noone will know. But I like to have everything right. That being said, the changes I proposed exactly match what was done in shipping with regard to making the rabbitmq host configurable. If I were to remove my commenting out of docker.init() and docker.spawn() so that my pull request only made the rabbitmq host configurable, would it be feasible to get that approved this week? I'll go ahead and do that in the hope that it will.
Thanks,
Roger

@rberlind
Copy link
Contributor Author

I went ahead and removed my commenting out of the docker spawning in the hopes that this might make it easier to merge my pull request with less review since the only change is now to make the rabbitmq host configurable exactly in the same way as is done in the shipping service.
Thanks

Copy link
Contributor

@jasonrichardsmith jasonrichardsmith left a comment

Choose a reason for hiding this comment

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

It appears to pass but you have no access to Coveralls token which is throwing an error. Otherwise it appears to be fine.

@jasonrichardsmith jasonrichardsmith merged commit 48d63b5 into microservices-demo:master Apr 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants