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 3 commits into
base: master
Choose a base branch
from

Conversation

ajharry69
Copy link

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. Make port-binding unique from another docker-compose service configured in the docker-compose.yaml file.

With these changes, running integration, lint and tox tests as configured in the docker-compose.yaml becomes possible.

What impacts?

The changes make the docker-compose configuration useful when running integration, lint and tox tests (through docker-compose up --build). When the configured lint checks are run, files are formatted per the pre-commit configuration.

@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.

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