Docker by 4c0n · Pull Request #66 · SyliusCrafts/BootstrapTheme

4c0n · 2020-07-18T16:23:23Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	#59
License	MIT

Progress on adding container setup.

4c0n · 2020-07-24T08:16:14Z

@pocky and anyone else of course. Feel free to test this and provide me with feedback :)
Like I said the initial building of twig templates will be slow, because of the theme template finder class that hits the database on every load/find template call, connecting fails because there is no db yet. Other than that it should be working, at least it is on my end 😄

pocky · 2020-07-28T21:11:49Z

Hello,

First feedback :)

Is it possible to add small instructions in README.md?
like I said on Slack, PHP version should be 7.3 because this is the current version in Sylius (I'm using 7.4 too but... rules are rules :D)
Blackfire? I understand why you want to add this container in your dev environment but this is not used by Sylius.
Build works but I think fixtures should be loaded for tests no?

ping @oallain and @jacquesbh

4c0n · 2020-07-28T21:43:04Z

Hi @pocky,

Thanks for the review!
Sure I'll update the README.
For PHP 7.3 I need to add a condition to the Dockerfile because some arguments differ, after that it can be controlled using the build args in the docker-compose file. Will do that soon.
The blackfire install is optional and disabled by default. It is very useful though.
Loading the fixtures is something you'd do after building and bringing the containers up. It's no different from Sylius standard. For your dev environment you'd have do that manually. Also the tests are cleanroom tests meaning that before every scenario the db is cleared and seeded just for that scenario.

How was the first build, slow?

…d internally

4c0n · 2020-08-03T14:44:04Z

Hi @pocky

Haven't heard back from you yet, I processed your feedback though 😄
Could you please have another look?

@oallain @jacquesbh,
Have you had any chance to look at this?

jacquesbh · 2020-08-05T10:08:58Z

I did a quick read but I really need more time to test the entire setup.

4c0n · 2020-08-05T12:38:02Z

Would be awesome if anyone could take some time to review/test it. For me it's working fine like I said before.
Kind of need this merged before moving on to the next step, which is selecting the theme by default without storing it in the database or by overriding the channel factory used in the tests to add the theme by default or something like that and then run all the tests.
At that last step we also should get a pretty good overview of what needs to be done to get all the tests working and can generate screenshots of all pages which we can check manually to see if the templates are all rendered correctly.
It would be of great value towards the goal of getting the semantic ui based layout replaced (at least on the shop end, guess admin will follow).

jacquesbh · 2020-08-05T12:50:36Z

@4c0n Is this relevant for your next step? :) Sylius/Sylius#11632

4c0n · 2020-08-05T13:05:47Z

@jacquesbh It is very useful for sure, I should include the env variable in this PR so that the fixtures will automatically add it for the dev environment.

For the tests the channel is added for each scenario through the behat setup ChannelContext. However @anthid figured out a solution for that: https://gist.github.com/anthid/dcc214804354c8d81013453d83aa219f already 👍

…theme to the channel automatically in the future

4c0n · 2020-08-11T07:47:11Z

@pocky ping

4c0n · 2020-08-31T12:02:21Z

Hi @pocky Hope you doing well 😃 Haven't heard from you in a while so I thought I'd follow up with you. I'd really like to move forward, but this PR is kind of a blocker for me as I explained before.

Looking forward to your response! Thanks

pocky

First, long time no see (and sorry).

Your work is awesome and it works with minor changes.

I requested some changes:

PHP should be 7.3 by default (yes I know :D)
vendors/composer
We don't need Blackfire for profiling in BootstrapTheme
I think it is possible to create something closer to PluginSkeleton https://github.com/Sylius/PluginSkeleton/tree/1.7 with a tests directory and a default Application

I also had errors with behat in behat.yml.dist and I made theses changes:

FriendsOfBehat\SymfonyExtension: 
   kernel:
       environment: "test"
        debug: true

Again, thanks and sorry for the lack of time.

pocky · 2020-11-09T22:58:08Z

.env.dist

+##################################
+# Blackfire profiling            #
+##################################
+# Set BLACKFIRE=true to enable   #
+# and configure values below.    #
+##################################
+BLACKFIRE=false
+BLACKFIRE_CLIENT_ID=
+BLACKFIRE_CLIENT_TOKEN=
+BLACKFIRE_SERVER_ID=
+BLACKFIRE_SERVER_TOKEN=
+BLACKFIRE_LOG_LEVEL=3


Is it possible to remove Blackfire?

It's possible, but like I said before it is optional and disabled by default. So why are you so set on removing it?

pocky · 2020-11-09T22:58:29Z

Dockerfile

@@ -0,0 +1,219 @@
+ARG PHP_VERSION=7.4


PHP_VERSION should be 7.3 by default

It is the default in the docker-compose.yml and env file, so this value is not used, but sure I can set it to 7.3, for more clarity.

Dockerfile

pocky · 2020-11-09T23:00:19Z

Dockerfile

+ARG BLACKFIRE=false
+
+RUN set -eux; \
+	if [ ! -z "$BLACKFIRE" ] && [ "$BLACKFIRE" = "true" ]; then \
+		curl -A "Docker" -o /tmp/blackfire-probe.tar.gz -D - -L -s https://blackfire.io/api/v1/releases/probe/php/alpine/amd64/$(php -r "echo PHP_MAJOR_VERSION.PHP_MINOR_VERSION;"); \
+		mkdir -p /tmp/blackfire; \
+		tar zxpf /tmp/blackfire-probe.tar.gz -C /tmp/blackfire; \
+		mv /tmp/blackfire/blackfire-*.so $(php -r "echo ini_get ('extension_dir');")/blackfire.so; \
+		printf "extension=blackfire.so\nblackfire.agent_socket=tcp://blackfire:8707\n" > $PHP_INI_DIR/conf.d/blackfire.ini; \
+		rm -rf /tmp/blackfire /tmp/blackfire-probe.tar.gz; \
+		mkdir -p /tmp/blackfire; \
+		curl -A "Docker" -L https://blackfire.io/api/v1/releases/client/linux_static/amd64 | tar zxp -C /tmp/blackfire; \
+		mv /tmp/blackfire/blackfire /usr/bin/blackfire; \
+		rm -Rf /tmp/blackfire; \
+	fi


Sorry for blackfire :). Could you remove this?

pocky · 2020-11-09T23:01:37Z

Dockerfile

+COPY --from=composer:latest /usr/bin/composer /usr/bin/composer
+
+# https://getcomposer.org/doc/03-cli.md#composer-allow-superuser
+ENV COMPOSER_ALLOW_SUPERUSER=1
+RUN set -eux; \
+	composer global require "hirak/prestissimo:^0.3" --prefer-dist --no-progress --no-suggest --classmap-authoritative; \
+	composer clear-cache
+ENV PATH="${PATH}:/root/.composer/vendor/bin"


It is impossible to use hirak/prestissimo with composer:2. I think we should use composer:1 instead of composer:latest (for Sylius 1.7)

Indeed, this was written before composer version 2 became the new "latest". I will make it easier to configure by introducing a new argument and using the version tag instead of the "latest" tag.

pocky · 2020-11-09T23:03:02Z

docker-compose.yml

+  php:
+    build:
+      args:
+        - BLACKFIRE=${BLACKFIRE:-false}


Remove this line

pocky · 2020-11-09T23:03:16Z

docker-compose.yml

+      - BLACKFIRE_CLIENT_ID=${BLACKFIRE_CLIENT_ID}
+      - BLACKFIRE_CLIENT_TOKEN=${BLACKFIRE_CLIENT_TOKEN}
+      - BLACKFIRE_SERVER_ID=${BLACKFIRE_SERVER_ID}
+      - BLACKFIRE_SERVER_TOKEN=${BLACKFIRE_SERVER_TOKEN}


Remove theses lines

pocky · 2020-11-09T23:06:03Z

Dockerfile

+RUN set -eux; \
+	yarn add \
+		@symfony/webpack-encore \
+		sass-loader@^8.0.0 \


Use ^9.0.0 instead of ^8.0.0

Okay, but I guess the instructions in the README and the docs need to be updated, as it states to do the following:
yarn add @symfony/webpack-encore sass-loader@^7.0.0 node-sass lodash.throttle -D
https://github.com/Sylius/BootstrapTheme/blob/master/README.md
https://docs.sylius.com/en/latest/book/themes/bootstrap-theme.html

Yes, I will made a PR asap.

pocky · 2020-11-09T23:06:21Z

Dockerfile

+	yarn add \
+		@symfony/webpack-encore \
+		sass-loader@^8.0.0 \
+		node-sass \


use node-sass@^4.0.0 instead of latest

pocky · 2020-11-09T23:09:24Z

README.md

+3. (Re)build the container images:
+    ```bash
+    docker-compose build
+    ```


Missing step:

Run behat
docker-compose exec php vendor/bin/behat --strict --tags="@javascript"

It is not missing, however it is beyond the scope of this PR. The intention was to add a docker setup. Running the tests is another task.

4c0n · 2020-11-11T12:31:24Z

First, long time no see (and sorry).

Apology accepted, you could of responded to my Slack messages and/or comments though. Even if it is just to say that you currently don't have the time to work on this.

We don't need Blackfire for profiling in BootstrapTheme

I already needed it, so I kind of disagree. Anyway it is an optional install (see README you need to specifically do something for it to be installed). So it really doesn't hurt to have the option of enabling it.

I think it is possible to create something closer to PluginSkeleton https://github.com/Sylius/PluginSkeleton/tree/1.7 with a tests directory and a default Application

I think you're right, it certainly is possible. But what would be the advantage? So let's say we want the theme to support both Sylius 1.7 and 1.8 from one theme version, then we can't use the test application, because it can only be one version. Also we aren't testing/executing any installation steps anymore that way.

I also had errors with behat in behat.yml.dist and I made theses changes:

FriendsOfBehat\SymfonyExtension: 
   kernel:
       environment: "test"
        debug: true

Running the tests is beyond the scope of this PR.

Again, thanks and sorry for the lack of time.

No worries, just as feedback, the lack of response was a much bigger issue for me than the lack of time.

pocky · 2020-11-11T20:43:09Z

Hello @4c0n,

About bootstrap and tests: my opinion is that this PR will allow us to test HTML rendered by BootstrapTheme. This is why I don't need Blackfire and I need to run Behat tests. Blackfire is useful for plugins and/or in the root application but not for a theme.

About tests (with Application) directory: I think we could tag a 1.0.0 for Sylius 1.7 and 2.0.0 for Sylius 1.8 OR 1.7.0 for Sylius 1.7 and 1.8.0 for Sylius 1.8.

jacquesbh · 2020-11-11T22:13:14Z

Hi!

I agree with @pocky that having Blackfire here is not a requirement and should be removed. Since this repo is community driven, it would be better not to add such changes if they are not needed.

Also, having a test application under tests/Application/ is a great idea! It could ease the whole setup.

I understand the use of Docker and we use a setting like this one in our own repositories, as example our makefile to run everything: https://github.com/monsieurbiz/SyliusColishipPlugin/blob/master/Makefile
It uses docker of course and the symfony binary to run the webserver. Actually it may be a nice thing to use, what do you think?

4c0n · 2020-11-12T09:38:48Z

@jacquesbh @pocky Without the profiler I would have had a very hard time finding out why the initial build was so slow for example, personally I think it is quite useful to find out what is going on. Also it is a development setup, not so much production or anything. But if you guys feel so strongly about it I will remove it. Could you point out an alternative solution to be able to profile that you do support, xdebug maybe?

Also, having a test application under tests/Application/ is a great idea! It could ease the whole setup.

All you need to do now to set this up is run docker-compose, how exactly does it ease the setup? I still don't see any advantage to this, it just seems more work.

makefile to run everything: https://github.com/monsieurbiz/SyliusColishipPlugin/blob/master/Makefile

I'm not a big fan of using make for this, that's personal though. Without it, all people need to do is use docker-compose with the right container and prefix the command they're used to using with "docker-compose exec [CONTAINER_NAME]`. Abstractifying that in a Makefile, for me makes little sense, but like I said that is personal. Maybe you can add it in a separate PR?

About tests (with Application) directory: I think we could tag a 1.0.0 for Sylius 1.7 and 2.0.0 for Sylius 1.8 OR 1.7.0 for Sylius 1.7 and 1.8.0 for Sylius 1.8.

Yes you could do that, but probably better to use branches, otherwise it'll be hard to fix older versions. Also you will have to maintain multiple versions, where that might not even be necessary. It would be good to decide on a versioning strategy for sure.
However I'm still looking for some advantage of having the test application packaged with the theme... I get that you want it, but not why you want it!

I'm not planning on spending a lot of time on this at the moment, as I got a lot of other stuff going on, so maybe you can just accept this as is, and make your own modifications after you merge it.

4c0n added 12 commits July 17, 2020 18:12

Added docker-compose file

da46fd3

Modified volume mounts

0d8e599

Using volume for public directory

176150f

Initial docker build setup

221b3a3

removed extra mount options

fbe2b9d

Using port 8080 for nginx

81d4ba4

Optional install of blackfire probe in the php container

21d5251

Fixed condition

8b0a4f4

Added ignore file

eb3b836

Using our own config file nginx

299ddfa

Environment section for php container

13db0c5

Placing logo url in src attribute of img element

4763b4e

4c0n changed the title ~~WIP: Docker~~ Docker Jul 18, 2020

This was referenced Jul 27, 2020

Add builds/CI #60

Closed

[WIP][PoC] add behat to github actions #71

Open

4c0n added 3 commits July 29, 2020 10:11

Added condition and build arg to build with PHP 7.3 by default

db80230

Fixed condition as variable get reassigned with micro version attache…

0822a80

…d internally

Added instructions on how to use container setup

a375bb6

Added environmental variable that will allow the fixtures to add the …

968149a

…theme to the channel automatically in the future

pocky requested changes Nov 9, 2020

View reviewed changes

4c0n closed this Apr 26, 2021

4c0n deleted the feature/docker branch April 26, 2021 22:28

Comments

Conversation

4c0n commented Jul 18, 2020

Uh oh!

4c0n commented Jul 24, 2020

Uh oh!

pocky commented Jul 28, 2020

Uh oh!

4c0n commented Jul 28, 2020

Uh oh!

4c0n commented Aug 3, 2020

Uh oh!

jacquesbh commented Aug 5, 2020

Uh oh!

4c0n commented Aug 5, 2020

Uh oh!

jacquesbh commented Aug 5, 2020

Uh oh!

4c0n commented Aug 5, 2020

Uh oh!

4c0n commented Aug 11, 2020

Uh oh!

4c0n commented Aug 31, 2020

Uh oh!

pocky left a comment

Choose a reason for hiding this comment

Uh oh!

pocky Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4c0n commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pocky commented Nov 11, 2020

Uh oh!

jacquesbh commented Nov 11, 2020

Uh oh!

4c0n commented Nov 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pocky Nov 9, 2020 •

edited

Loading

4c0n commented Nov 11, 2020 •

edited

Loading