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 database to host machine so it can be easily inspected #10

Merged
merged 2 commits into from
Mar 15, 2021

Conversation

adamabeshouse
Copy link
Contributor

@adamabeshouse adamabeshouse commented Mar 9, 2021

Also add .gitignore

Comment on lines 25 to 26
ports:
- "3306:3306"
Copy link
Member

Choose a reason for hiding this comment

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

for production use often one prolly wouldn't want to expose 3306 because it opens up the port to any process on the machine. This is protected atm by running this cbioportal-database container within cbio-net network (see networks section). That is, only containers on this network are able to access 3306. To connect to the db with the mysql command you can either use exec to run a mysql command within the cbioportal-database container. Or you can start a separate mysql container on the cbio-net. Not sure if those alternatives cover your use case?

Copy link
Member

@inodb inodb Mar 9, 2021

Choose a reason for hiding this comment

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

Discussed some more with @adamabeshouse . Use case is connecting to sequel pro locally for dev use. Can't see how to do that w/o exposing the port

@pvannierop @nr23730 any thoughts on exposing this port? I figured maybe it's not that big of a risk. Alternatively maybe we should add some dev flag (or dev compose file)

Copy link
Contributor

@nr23730 nr23730 Mar 10, 2021

Choose a reason for hiding this comment

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

I would propose to add a separate -dev compose file that may also do the following:

  • Enable Debug for init script (SHOW_DEBUG_INFO: "true")
  • Expose MySQL port
  • Expose session service port
  • Expose MongoDB port for session service

Also I may add a PR that easily allows so set a different port for cBioPortal as 8080 is very common in may already be in use. Of course it's possible for everyone to change the compose file, but that might cause unnecessary merge conflicts.

Copy link
Contributor

@pvannierop pvannierop Mar 10, 2021

Choose a reason for hiding this comment

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

See: https://docs.docker.com/compose/extends/

This can be best done with a compose extension file like so:

db-ports.yml

services:
   cbioportal-database-container:
   ports:
      - "3306:3306"

And then run:

docker-compose -f docker-compose.yml -f db-ports.yml up -d

Copy link
Contributor

@pvannierop pvannierop left a comment

Choose a reason for hiding this comment

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

See: https://docs.docker.com/compose/extends/

I propose to use a compose extension strategy that will leave the docker-compose.yml unchanged.

Comment on lines 25 to 26
ports:
- "3306:3306"
Copy link
Contributor

@pvannierop pvannierop Mar 10, 2021

Choose a reason for hiding this comment

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

See: https://docs.docker.com/compose/extends/

This can be best done with a compose extension file like so:

db-ports.yml

services:
   cbioportal-database-container:
   ports:
      - "3306:3306"

And then run:

docker-compose -f docker-compose.yml -f db-ports.yml up -d

Signed-off-by: Adam Abeshouse <abeshoua@mskcc.org>
@adamabeshouse adamabeshouse force-pushed the port branch 2 times, most recently from 1d48a8b to d242b25 Compare March 12, 2021 21:10
@adamabeshouse
Copy link
Contributor Author

adamabeshouse commented Mar 12, 2021

@inodb @pvannierop @nr23730 Thanks for your comments. I have made requested changes, let me know if it looks good

Copy link
Contributor

@nr23730 nr23730 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
Contributor

@pvannierop pvannierop left a comment

Choose a reason for hiding this comment

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

Yes, this looks good! Minor gripe I have is with the filename. I would think that open-db-ports.yml would be better. When the idea is to open ports for other services as well in the future, I propose to leave the filename as is.

Signed-off-by: Adam Abeshouse <abeshoua@mskcc.org>
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.

4 participants