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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-30036: Add Node.js and Yarn to 7.1 and 7.2 images #33

Merged
merged 6 commits into from Feb 5, 2019

Conversation

4 participants
@mnocon
Copy link
Member

mnocon commented Jan 31, 2019

After ezsystems/ezplatform#345 is merged server requires both Node.js and Yarn to run eZ Platform.

Currently they are not available in our Docker images, meaning that Travis will stop working when it's merged - example failure can be seen in ezsystems/ezplatform-admin-ui#815 (it's ezsystems/ezplatform-admin-ui#796 set to use ezsystems/ezplatform#345 as metarepo with Node and Yarn installed on Travis).

I think we need to add these packages to our images. I don't like the fact that that everything about the images here screams PHP and I'm adding JS, but I'm not sure how else this problem can be solved - so I welcome all suggestions how to approach this 馃槈

I've tested the images with bin/.travis/build.sh 7.1 and bin/.travis/build.sh 7.2, looks like they are created correctly.

@mnocon

This comment has been minimized.

Copy link
Member Author

mnocon commented Jan 31, 2019

@andrerom @vidarl I can't add you as reviewers here - what do you think, how should we approach this?

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jan 31, 2019

hmm....

Ideally I'd rather have separate image/container for this, or as part of dev php image.
otherwise we end up going in direction of "everything and the kitchen sink" container 馃槃

@mnocon

This comment has been minimized.

Copy link
Member Author

mnocon commented Jan 31, 2019

Yep, I'm not a fan of this as well. I don't like adding it to the dev image only as this is a requirement for production (and seems misleading).

So, if I understand correctly: we need to have an image with both PHP and Node, they cannot be separated. Do we want to publish the images under different names? We have ezsystems/php:7.2-v1, something like ezsystems/php-node:7.2-v1?

I could create a new Dockerfile that adds Node, similar to the current dev file, modify build script to accept additional argument (--with-node) so it could look like this:
bin/.travis/build.sh 7.1 -> no change in behaviour
bin/.travis/build.sh 7.1 --with-node -> builds ez_php:latest and ez_php:latest-dev with Node (I think it should be possible to extend ez_php:latest and overwrite it?)
And:
bin/.travis/push.sh ezsystems/php-node to publish the image.

Does it make sense?

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jan 31, 2019

well.. do we use app image in the cases you need this?
If so we could also just add it here: https://github.com/ezsystems/ezplatform/blob/master/doc/docker/Dockerfile-app

@mnocon

This comment has been minimized.

Copy link
Member Author

mnocon commented Jan 31, 2019

Yes, we do - I thought about this, but won't it be executed every time a job is started, compared to executing it once (when tagging) and then downloading a ready image?

Not sure about the amount of time this could potentially save, if you think it's the way to go I can do this (as it is indeed clearer)

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jan 31, 2019

What's the cost in terms of size of this image?
I see Plopix installs node.js on Launchpad for php image, so I guess it can make sense that we do it here.

@mnocon

This comment has been minimized.

Copy link
Member Author

mnocon commented Feb 1, 2019

For PHP 7.2, generated with changes in this PR (with docker images)

REPOSITORY                         TAG                         SIZE
ez_php                             latest-dev                  694MB
ez_php                             latest                      581MB

Compared to current values for php-7.2-v1 (162, 200 - per https://hub.docker.com/r/ezsystems/php/tags) it's a huge increase 馃 Not sure how to optimise, as I'm just installing packages and all the unnecessary stuff should be removed in the last stage (https://github.com/ezsystems/docker-php/blob/master/php/Dockerfile-7.2#L88)?

@@ -89,7 +97,8 @@ RUN set -xe \
&& pecl clear-cache \
&& rm -Rf "$(pecl config-get temp_dir)/*" \
&& docker-php-source delete \
&& apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false $buildDeps
&& apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false $buildDeps \
&& apt-get clean

This comment has been minimized.

@andrerom

andrerom Feb 1, 2019

Member

I might remember this wrong, but this might have been skipped on purpose so Docker-app does not need to start with apt-get update -q -y as well. However if that is a good thing or not is another question, it's just a break if this is removing update definitions, meaning it will need a version bump in format version string.

This comment has been minimized.

@mnocon

mnocon Feb 1, 2019

Author Member

You're probably right, the third commit was me going "Docker Hub reports that Plopix's PHP+Node is only 359 MB, wonder how he achieved that" (and copying stuff from https://github.com/Plopix/docker-php-ez-engine/blob/master/php72/Dockerfile to test). Interestingly it made no difference in image size, I'll revert that.

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Feb 1, 2019

Compared to current values for php-7.2-v1 (162, 200 - per https://hub.docker.com/r/ezsystems/php/tags) it's a huge increase

I think Docker hub is counting this differently maybe (only unique layer, not counting what it builds on afaik), so best is to do a test build on master to compare to make sure we are comparing 馃崕's.

@mnocon

This comment has been minimized.

Copy link
Member Author

mnocon commented Feb 1, 2019

Results from master:

REPOSITORY                         TAG                 SIZE
ez_php                             latest-dev          561MB
ez_php                             latest              447MB

looks like it's a ~140mb increase in image size for PHP 7.2

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Feb 1, 2019

ok

@vidarl What do you think? I'm leaning towards one of:
A. As @mnocon suggests, tag own flavour with this (own -node tag), make dev extend that instead so it has all
B. Just add to dev image

Either way we will also need to:

So I might lean towards A.

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Feb 4, 2019

@vidarl are you OK with proposed changes? We need to make a decision anytime soon as it is holding us back on testing Webpack Encore integration.

@vidarl vidarl self-requested a review Feb 5, 2019

@vidarl
Copy link
Member

vidarl left a comment

I like Andre's suggestion ( option A as he call it ) : Dedicated -node tag which we use during the build stage and in -dev image

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Feb 5, 2019

@mnocon could you address this today so we can build those images as soon as possible?

@mnocon

This comment has been minimized.

Copy link
Member Author

mnocon commented Feb 5, 2019

@lserwatka yes, working on it

@mnocon mnocon force-pushed the mnocon:EZP-30036-webpack-encore branch 2 times, most recently from 96533a6 to df48c3a Feb 5, 2019

@mnocon mnocon force-pushed the mnocon:EZP-30036-webpack-encore branch from df48c3a to b1415b9 Feb 5, 2019

@mnocon

This comment has been minimized.

Copy link
Member Author

mnocon commented Feb 5, 2019

@andrerom @vidarl pushed new approach - added new Dockerfile that adds Node to latest and latest- dev if --with-node is specified (no changes otherwise). Didn't want to copy current Dockerfiles to create something like Dockerfile-7.2-node - tested with PHP 7.1 and PHP 7.2 and it works.

I think the images can be published under ezsystems/php-node. Is it ok? if you think we should keep them under php - how should the naming schema look (as I'll have to modify push.sh to take that into account) php-MAJOR.MINOR-node-VERSION?

If you think this is ok I'll create a PR to metarepo, changing these lines:
https://github.com/ezsystems/ezplatform/blob/master/.env#L13
to:

PHP_IMAGE=ezsystems/php-node:7.2-v1
PHP_IMAGE_DEV=ezsystems/php-node:7.2-v1-dev
@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Feb 5, 2019

I would rather keep it on same repo, so that dependency on each other looks always like this:
php: -> php:-node -> php:-dev

This means you can drop your .travis.yml changes and just adapt bin scripts* to take this into account. Aka so we always build node "flavour", and dev always extend node.

* First and foremost build and push, test already checks dev so will detect if something is also wrong with node because it will extend it, but you could add minimal test to see if node/yarn is working.

@mnocon mnocon force-pushed the mnocon:EZP-30036-webpack-encore branch from 921bc21 to cd34516 Feb 5, 2019

@mnocon mnocon force-pushed the mnocon:EZP-30036-webpack-encore branch from cd34516 to 59ae5d1 Feb 5, 2019

@mnocon

This comment has been minimized.

Copy link
Member Author

mnocon commented Feb 5, 2019

Changed:
latest, latest-node and latest-dev are always built now (and the dependency chain is normal -> node -> dev).

Added tagging the node flavour, the formats are:

ezsystems/php                      7.2-node
ezsystems/php                      7.2-v1-node

(and as previously)
ezsystems/php                      7.2-dev
ezsystems/php                      7.2-v1-dev
ezsystems/php                      7.2       
ezsystems/php                      7.2-v1

Added basic README entry about Node and added tests that check that node and yarn commands are available in test and dev images. Waiting for Travis results 馃檪

@mnocon

This comment has been minimized.

Copy link
Member Author

mnocon commented Feb 5, 2019

@andrerom @vidarl looks like Travis agrees - please have a look again

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Feb 5, 2019

I know you need this for several other things, so merging as it seem clean and green and follows also what @vidarl prefered.

@andrerom andrerom merged commit 11f9678 into ezsystems:master Feb 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment