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

Refactor: Docker #342

Merged
merged 9 commits into from
Nov 9, 2024
Merged

Refactor: Docker #342

merged 9 commits into from
Nov 9, 2024

Conversation

alexskr
Copy link
Member

@alexskr alexskr commented Oct 30, 2024

Dockerfile:

  • Change ruby image OS from Alpine to Debian slim-bookworm. Alpine is effective for reducing image size; however, using the slim version of Debian achieves a similar reduction. We can further reduce image size by adopting staged builds and moving Node/Yarn into a separate build stage.
  • Use ruby 3.2
  • Removed docker binaries from the image.
  • Set bundler gem path to /usr/local/bundle docker-compose:
  • Add internal/external networks
  • Make node a non-runtime dependency
  • Do not expose ports for caching/db by default config:
  • Update sample env config file, remove outdated settings, and add support for looking up some variables from ENV variables if needed.

Dockerfile:
* Change ruby image OS from Alpine to Debian slim-bookworm.
  Alpine is effective for reducing image size; however, using the slim version
  of Debian achieves a similar reduction. We can further reduce image size
  by adopting staged builds and moving Node/Yarn into a separate build stage.
* Use ruby 3.2
* Removed docker binaries from the image.
* Set bundler gem path to /usr/local/bundle
docker-compose:
* Add internal/external networks
* Make node a non-runtime  dependency
* Do not expose ports for caching/db by default
config:
* Add development and test config files.  Config files are redesigend to be
  used with .env so is safe to add
Copy link
Contributor

@syphax-bouazzouni syphax-bouazzouni left a comment

Choose a reason for hiding this comment

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

👍

@jvendetti
Copy link
Member

Why are you adding BioPortal specific config files to the source tree? And why do they contain a lot of global variables that aren't specific to functionality that currently exists in BioPortal, e.g., $FAIRNESS_DISABLED, $FAIRNESS_URL, $OMNIAUTH_PROVIDERS, $HOME_PAGE_LOGOS, etc. I disagree with introducing global variables for functionality we don't serve in our environment.

@alexskr
Copy link
Member Author

alexskr commented Nov 5, 2024

The development and test config files have been added because they are necessary for running the application in Docker containers during development and testing. The intention is to have common settings included in the config files, and any unique/site-specific or sensitive configurations to be handled separately in the .env file, which is picked up in docker-compose.yaml

Config files should be cleaned up, and configuration for features not present in the code base can be removed. We also have a sub-task outlined for it in the #338

@jvendetti
Copy link
Member

I don't run the BioPortal application in a Docker container for doing local development. I don't agree with the modifications you made to bioportal_config_development.rb, which if merged to master breaks my development environment.

The explanation about adoption of an .env file is insufficient. The use of .env files in modern Rails applications is out of favor. Sensitive configuration information belongs in encrypted credentials, and common settings should go in YAML files which are accessible at runtime with the Rails configuration API. I've been gradually refactoring the application in that direction, which you can see examples of in files like settings.yml and change_request.yml.

@alexskr
Copy link
Member Author

alexskr commented Nov 6, 2024

How would you propose passing in config variables into the docker container at run time without the use of environmental variables?

@jvendetti
Copy link
Member

I don't have time to break from working on my obligations for the U24 grant to figure out how Docker support should be integrated into BioPortal's codebase. What I'm asking is that it's done in such a way that you don't invalidate the current methodology that we use for doing local development.

sample config file changes:
 - remove unused/obsolete settings like error email notifications,
   MAX_CHILDREN, MAX_POSSIBLE_DISPLAY and not yet implemented features
 - remove Google Analytics ID because it is moded to encypted credentials
 - use env variables with default values

remove development and test config files, create them in docker image
   during build process.
@alexskr alexskr marked this pull request as ready for review November 7, 2024 21:24
@alexskr alexskr self-assigned this Nov 7, 2024
@alexskr
Copy link
Member Author

alexskr commented Nov 9, 2024

bioportal_config_development.rb files are removed

@alexskr alexskr merged commit e6fd546 into sync-agroportal Nov 9, 2024
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