-
Notifications
You must be signed in to change notification settings - Fork 312
adding rabbitmq container #51
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
Conversation
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.
Nice! Added some comments inline.
return parameters | ||
|
||
@wait_container_is_ready() | ||
def declare_queue(self, queue): |
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.
This seems to be a duplicate (in terms of signature) of the function in line 48. Should this be removed?
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.
according to the logic - this method is supposed to declare multiple queues
tests/test_rabbitmq.py
Outdated
import pika | ||
|
||
def test_declare_queue(): | ||
rabbitmq_container = RabbitmqContainer("rabbitmq:3-management") |
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.
Why can we not use the standard rabbitmq:lastest
tag here?
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.
because it does not work with rabbitmq:latest
:)
@dabrign any plans to finish it? if no - I'll take care of this pr |
|
||
@wait_container_is_ready() | ||
def declare_vhost(self, vhost): | ||
cmd = "rabbitmqadmin declare exchange vhost={} type={}".format(vhost) |
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.
type is missing
RABBITMQ_DEFAULT_PASS = os.environ.get("RABBITMQ_DEFAULT_PASS", "guest") | ||
SERVICE_STARTED = r"Server startup complete;" | ||
|
||
def __init__(self, image="rabbitmq:latest"): |
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.
rabbitmq:latest
does not have rabbitmqadmin
, so all methods below which are using it do not work
@wait_container_is_ready() | ||
def get_connection_parameters(self): | ||
credentials = pika.PlainCredentials(self.RABBITMQ_DEFAULT_USER, self.RABBITMQ_DEFAULT_PASS) | ||
parameters = pika.ConnectionParameters(self.get_container_host_ip(), self.port_to_expose, |
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.
self.port_to_expose
-> self.get_exposed_port(self.port_to_expose)
self.with_exposed_ports(self.port_to_expose) | ||
self.queues = list() | ||
|
||
def _configure(self): |
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.
this method is not called at all, nowhere
RabbitMQ support was added in #162 |
This pull request is meant to add rabbitmq to this suite. However, it is still a 'work-in-progress', but I would like to have some opinions on this implementation and, specifically, on how to write an efficient test.
More in details, it is well known that the decorator
@wait_container_is_ready
just wait if is possible to 'talk' with the container and it does not wait until the application is ready to receive commands.For that reason, in order to successfully exit the test, I had to add a (very inelegant) sleep to wait the additional time needed for the application inside the container to be ready.
Anyone experienced similar problem?
How did you solve it?
Thanks