Skip to content
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

Add missing docker configurations #80

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

ajharry69
Copy link

@ajharry69 ajharry69 commented Nov 19, 2024

What is being delivered?

  1. Added a Dockerfile for use in docker-compose.yaml file.
  2. Removed reference to non-existent Dockerfile in the docker-compose.yaml file build context.
  3. Add Make targets for conveniently running integration tests for both RabbitMQ and ActiveMQ.
  4. Decouple STOMP server host user from local environment by retrieving the host from settings.STOMP_SERVER_HOST.

What impacts?

With these changes, running tests and lint checks as configured in the docker-compose.yaml becomes is possible without manually installing dependencies in your local development environment. You simply run make test and let docker (compose) do the rest.

@ajharry69 ajharry69 requested a review from a team as a code owner November 19, 2024 09:54
@@ -72,7 +71,7 @@ services:
broker-rabbitmq:
image: rabbitmq:3-management
ports:
- 61613:61613
Copy link
Member

Choose a reason for hiding this comment

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

Port 61613 is used inside the code. The pipeline probably broke because of this. Do you really need to change the port?
The idea would be to start one container at a time and not both at the same time.
Did this configuration work locally?

Copy link
Author

Choose a reason for hiding this comment

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

I started all the services at the same time (using docker compose up [--build]), hence the reason it was reporting a port already in use error. I can:

  1. revert the change and leave clear guidelines on the README about how to run the tests in docker compose setup, OR
  2. change the rabbitmq port in the code to support both cases.

I am not sure how detrimental the 2nd option will be, but a person new to the repo like I am will expect it not to fail if they ran the same command.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's this section that says to run the tests with either broker-activemq or broker-rabbitmq. If it's not clear enough, it's an opportunity to improve it

Copy link
Author

Choose a reason for hiding this comment

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

There's this section that says to run the tests with either broker-activemq or broker-rabbitmq. If it's not clear enough, it's an opportunity to improve it

I'll have a closer look at how to better approach this, then update ASAP.

Copy link
Author

Choose a reason for hiding this comment

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

@ricardochaves , I am not sure why the pipeline is still failing despite rolling back the port change. I successfully ran tox in my local setup as per the documentation. Checking the Azure pipeline reports seems to require privileged access, or I may not be going about it correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants