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 alternate configuration so "web" images (app.jar) can be loaded in docker #26

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

pappde
Copy link

@pappde pappde commented Aug 21, 2024

ISSUE

Here is a dev experience following the documentation:

  1. setup docker
  2. cbioportal repo: use docker to build "web-and-data" image (with loose files)
  3. cbioportal-docker-compose repo: run "docker compose up"
  4. observe works

If the dev wants to test with the "app.jar" image, which mirrors the actual production deployment, the steps would not work:

  1. cbioportal repo: use docker to build "web" image (with loose files)
  2. cbioportal-docker-compose repo: run "docker compose up"
  3. observe error about PortalApplication missing in docker container log

FIX

This PR adds an alternate docker configuration (YML) that instead loads the image and executes the app.jar. So the user needs to do:

docker compose -f docker-compose.web.yml up

RELATED

Still need to update the cbioportal repo documentation. That will be a separate PR.

FURTHER WORK

Copy-pasting the docker-compose.yml is not very maintainable. There is probably much better solution to this that someone familiar with docker configurations can address.

For example, is the "command:" line really necessary, since each image has it's own Dockerfile that can define what should be executed? Is it only in place for the "rm servlet-api-2.5.jar" hack? Is that still necessary?

REVIEW

@alisman

Denis Papp added 2 commits August 21, 2024 14:22
EXPL: the existing docker configuration only works with the "web-and-data" image of cbioportal. This updates it to run the app.jar
NOTE: the user must use "docker compose -f ... up"
Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

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

@pappde thanks for these fixes! Very helpful! Could you maybe move the yaml file under dev/ somewhere? Feels like this is bit more of an advanced use case

I think you can avoid copying some of the Docker Compose file by supplying multiple docker compose files with -f file1 -f file2. E.g. similar to how it is done for open-ports: docker compose -f docker-compose.yml -f open-ports.yml up. It only overrides the couple of fields that need to be changed. Hope that helps!

Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!

@zainasir zainasir merged commit df178da into cBioPortal:master Nov 11, 2024
@pappde pappde deleted the fix/web-image branch November 11, 2024 22:11
@pappde
Copy link
Author

pappde commented Nov 11, 2024

Thanks for updating - this is much cleaner.

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