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

Expose postgres DB on port 25432 instead of 5432 #540

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

epapbak
Copy link
Collaborator

@epapbak epapbak commented Nov 22, 2023

Description

The exposed port is not used by any of the automation of this repository. Instead, it is just used for debugging when needed. Let's use 25432 so it does not conflict with any locally running PSQL server

Type of change

  • Refactor (refactoring code, removing useless files)

Testing steps

Tested locally

Checklist

  • Pylint passes for Python sources
  • sources has been pre-processed by Black
  • updated documentation wherever necessary
  • new tests can be executed both locally and within docker container
  • new tests have been included in scenario list (make update-scenarios)

Copy link
Collaborator

@Bee-lee Bee-lee left a comment

Choose a reason for hiding this comment

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

ok

Copy link
Collaborator

@joselsegura joselsegura left a comment

Choose a reason for hiding this comment

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

Please, don't merge.

I'm not sure about the port is not used by the automation. IIRC some of the scripts define a bunch of environment variables and maybe there the potr is being defined

@joselsegura
Copy link
Collaborator

I just reviewed and I can see that the port is used in:

  • Dockerfile, as environment variable that will be used by any process running the container image
  • Lot of *.toml configuration files for the programs under test
  • The environment variable defined in Dockerfile is being used by Behave Python code (environment.py)

Copy link
Collaborator

@tisnik tisnik left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@joselsegura joselsegura left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't realize that for docker-compose containers de port will remain untouched, it's just a change if you try to connect from the host.

Sorry for delaying you

@epapbak
Copy link
Collaborator Author

epapbak commented Nov 22, 2023

I just reviewed and I can see that the port is used in:

  • Dockerfile, as environment variable that will be used by any process running the container image
  • Lot of *.toml configuration files for the programs under test
  • The environment variable defined in Dockerfile is being used by Behave Python code (environment.py)

@joselsegura I think there's a misunderstanding. When using the port keyword and not the expose keyword in the docker-compose file, the format is HOST:CONTAINER, the services connect between each other using the value defined on the right, which represents the container's port. The other value would be used to access a container from the host machine. So this should work fine. But I think I'll do more testing, especially in the CI env, before merging, so we're more sure of this change =)

@epapbak epapbak merged commit c820d82 into RedHatInsights:main Nov 22, 2023
@epapbak
Copy link
Collaborator Author

epapbak commented Nov 22, 2023

tested by running the tests within docker and no local postgres running

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.

5 participants