-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
Single Dockerfile using multi-stages for production and dev image #751
Open
AlMaVizca
wants to merge
5
commits into
farmOS:3.x
Choose a base branch
from
AlMaVizca:3.x-docker-multi-stage
base: 3.x
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8fe46ef
Define baseimage with arguments required in multiple stages
AlMaVizca ee9f49b
Avoid repeating common path
AlMaVizca a248e17
Refactor composer dependencies to create the production image
AlMaVizca 682fdbb
Relocate php unit configuration
AlMaVizca 8320482
Merge development image into a single Dockerfile
AlMaVizca File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
#!/usr/bin/env sh | ||
|
||
cp -p ${FARMOS_PATH}/web/core/phpunit.xml.dist ${FARMOS_PATH}/phpunit.xml | ||
sed -i 's|bootstrap="tests/bootstrap.php"|bootstrap="web/core/tests/bootstrap.php"|g' ${FARMOS_PATH}/phpunit.xml | ||
sed -i '/failOnWarning="true"/a \ failOnIncomplete="true"' ${FARMOS_PATH}/phpunit.xml | ||
sed -i '/failOnWarning="true"/a \ failOnSkipped="true"' ${FARMOS_PATH}/phpunit.xml | ||
sed -i 's|name="SIMPLETEST_BASE_URL" value=""|name="SIMPLETEST_BASE_URL" value="http://www"|g' ${FARMOS_PATH}/phpunit.xml | ||
sed -i 's|name="SIMPLETEST_DB" value=""|name="SIMPLETEST_DB" value="pgsql://farm:farm@db/farm"|g' ${FARMOS_PATH}/phpunit.xml | ||
sed -i 's|name="BROWSERTEST_OUTPUT_DIRECTORY" value=""|name="BROWSERTEST_OUTPUT_DIRECTORY" value="/var/www/html/sites/simpletest/browser_output"|g' ${FARMOS_PATH}/phpunit.xml | ||
sed -i 's|name="MINK_DRIVER_ARGS_WEBDRIVER" value='\'''\''|name="MINK_DRIVER_ARGS_WEBDRIVER" value='\''["chrome", { "chromeOptions": { "w3c": false, "args": ["--disable-gpu","--headless", "--no-sandbox"] } }, "http://chrome:4444/wd/hub"]'\''|g' ${FARMOS_PATH}/phpunit.xml | ||
sed -i 's|\./|\./web/core/|g' ${FARMOS_PATH}/phpunit.xml | ||
sed -i 's|\.\./web/core/|\./web/|g' ${FARMOS_PATH}/phpunit.xml | ||
sed -i 's| </php>| <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled"/>'"\n"' </php>|g' ${FARMOS_PATH}/phpunit.xml | ||
|
||
# Create output directory for phpunit tests and permissions for testing user. | ||
mkdir -p ${FARMOS_PATH}/web/sites/simpletest/browser_output | ||
chown -R www-data:www-data ${FARMOS_PATH}/web/sites/simpletest | ||
|
||
rm ${FARMOS_PATH}/phpunit.sh |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git
andunzip
are currently installed and available only in stages related to building farmOS. In my opinion that they should also be available at least in the final dev version of the image but probably also in the final prod image since they are required to install additional modules using composer which is a workflow available in current image versions, so removing it here would be a breaking change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, when installing them in the stage that will be included in the final image, it would be good to add
--no-install-recommends
option to theapt-get install
command to skip installing additional packages that are not really needed and only take up space in the image. It is a good practice to use this option in all instances of apt-get in docker images.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you, please, share a module that requires some of those packages?
I've tried with a default installation, also adding modules that are displayed during setup, and later on
/setup/modules
and they were installed without errors.I understand that those packages might be useful for development image. Yet, for production, I think that adding any module without creating a new image for distribution, or without custom shared volumes, might create a scenario where if the container is replaced, it will lose functionality.
Since there is no documentation about shared volumes to persist modules information, I think, it is not a case to resolve in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to minimize the number of changes that has to do with packages installations, and propose a different structural organization.
I took the time to evaluate which packages are installed by recommendations, and only
git
andunzip
are adding extra packages. Would it be enough to add them only to the dev image? Is it worth it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of community modules, not included in the farmOS itself. For example https://github.com/symbioquine/farmOS_asset_link or https://www.drupal.org/project/farm_map_custom_layers.
On the Building farmOS with Composer documentation page, there is described approach for adding additional community modules to the composer.json and building docker image based on official production one with these customizations. Currently, in instructions for building a custom image when it comes to installing composer dependencies only
composer install
is provided, without git and unzip in the prod image these instructions will no longer work and will need to be updated to also include installation of git and unzip. So not including git and uzip in production image may break some existing setups when the new farmOS base image version will be pulled.I would include git and unzip in both dev and prod image to don't break existing documented functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, although, I think that it would be more appropriate to add the information on the documentation, so the image is kept as slim as possible, It's the right thing to keep it working as it was.
Thanks for the explanation.