various improvements and build updates #130

dsifford · 2018-03-12T19:52:41Z

This PR removes several redundancies and introduces several improvements to the overall build process.

Notable changes are as follows:

Simplify everything down into a single Dockerfile by using build args.
Remove certbot from the wordpress image completely as this is better suited as a separate service using the official certbot image.
Create and use a non-root user (admin) with passwordless sudo priviledges, rather than using the root user.
Readdress Usability and readability - when errors happen with plugin install #110 to print out logs in a similar, but greatly simplified, format
Install and remove themes and plugins in parallel.
Various simplifications across the board in run.sh.
Format run.sh file automatically with shfmt.

Stuff still left to do after review:

Remove commented out lines in Dockerfile
Update changelog
Version bump
Build and push to Dockerhub

Let me know what you think @karellm

karellm

Thanks for this update. I did a first pass but most changes look good. I will try it on our install shortly too.

karellm · 2018-03-14T17:17:32Z

Dockerfile

-    && mkdir -p /app ~/.wp-cli \
+    && service apache2 restart
+
+# Install wp-cli, add scripts, create install directory & symlink


This doesn't install wp-cli anymore, it's done above

Good catch.

karellm · 2018-03-14T17:18:00Z

Dockerfile


 # Install base requirements & sensible defaults + required PHP extensions
-RUN echo "deb http://ftp.debian.org/debian stretch-backports main" >> /etc/apt/sources.list \
-    && apt-get update \
+# RUN echo "deb http://ftp.debian.org/debian $(sed -n 's/^VERSION=.*(\(.*\)).*/\1/p' /etc/os-release)-backports main" >> /etc/apt/sources.list \


Do you want to keep this? I don't think commented out code should stay.

Yep, agreed. That's what the first checkbox in the PR was addressing.

Will fix.

karellm · 2018-03-14T17:19:07Z

Dockerfile

-    && DEBIAN_FRONTEND=noninteractive apt-get -t stretch-backports install -y \
-        python-certbot-apache \
+    # && DEBIAN_FRONTEND=noninteractive apt-get -t stretch-backports install -y \
+    #     python-certbot-apache \


Do you want to keep this? I don't think commented out code should stay.

Same as above comment. Will Fix.

karellm · 2018-03-14T17:23:11Z

build.sh

+for php_version in "${php_versions[@]}"; do
+	docker build \
+		--build-arg PHP_VERSION="$php_version" \
+        --build-arg VERSION="$npm_package_version" \


extra space

Whoops. shfmt might have added that break.

Will fix.

karellm · 2018-03-14T17:25:51Z

build.sh

+		--build-arg PHP_VERSION="$php_version" \
+        --build-arg VERSION="$npm_package_version" \
+		-t "visiblevc/wordpress:latest" \
+		-t "visiblevc/wordpress:latest-php7.2" \


I think this won't work, shouldn't it use the $php_version version? Also we don't want to set latest for all version, only for 7.2

It should work I think unless I'm overlooking something.

The PHP_VERSION arg is the one that sets the base image in the Dockerfile.

The npm_package_version arg is only used for setting the version label in the Dockerfile.

I see what you mean Re: your latest concern. I'll add a fix for that so that only the latest php version image gets tagged with latest.

Oh hah... Now I'm also noticing that 7.2 is hard-coded in there still.. Fixing that too 😊

karellm · 2018-03-14T17:27:54Z

php7.0/Dockerfile

-        soap \
-        zip \
-    # See https://secure.php.net/manual/en/opcache.installation.php
-    && echo 'memory_limit = 512M' > /usr/local/etc/php/php.ini \


Don't we want to port this too?

That's on line 33 of the new Dockerfile

karellm · 2018-03-14T17:33:42Z

run.sh

-  fi
+configure() {
+    # Ensures that this only runs on the initial build
+    [[ -f ~/.wp-cli/config.yml ]] && return


I'm not sure this take any time so I would run it everytime. It allows that changes to variables are actually passed down to the image. wdyt?

I see what you mean.

The issue currently is that it would append the ServerName directive and append the completions line in the bashrc on every restart.

So the end result of that would be a single ServerName, but a bunch of ServerAliases and a bunch of . /etc/bash_completion.d/wp-clis.

I can prob adjust the logic there though if you think we should keep the ability to change variable defaults.

karellm · 2018-03-14T17:37:24Z

run.sh

-  PLUGINS="${PLUGINS/%,},"
-  THEMES="${THEMES/%,},"
-
-  if [[ -f /root/.dockercache ]]; then


Is there a reason you removed this?

Crap. Good catch.

Forgot that we needed to keep track of those beyond deciphering which of the existing themes/plugins are volumed.

I added the new --skip-content flag to the wp core download call on line 95 and I thought that would satisfy our needs.

The good news is that, even though we need to add this back in, it can be greatly simplified with skip-content enabled.

I'll add that back in.

dsifford · 2018-03-14T23:12:42Z

@karellm should be good now

karellm · 2018-03-15T15:05:18Z

@dsifford Would you mind including these:

composer (close Add composer to the container, fix #74 #76)
reverse proxy (close Addition to wp-config.php to allow https via reverse proxy #106)
support for locale (close feat(locale): add WP_LOCALE environment variable #124)

dsifford · 2018-03-15T16:51:17Z

@karellm Do you think we should be adding composer to the image? Seems kind of heavy considering most users won't be using it or needing it.

karellm · 2018-03-15T18:04:12Z

@dsifford I don't need it personally but I can see why it is a reasonable request. Can you think of an alternative solution?

dsifford · 2018-03-15T18:23:50Z

@karellm I probably could if I had an idea of what the common use-case is for it in wordpress projects.

Do people just use it to bundle PHP libraries with their themes/plugins? Do they use it to literally manage their themes/plugins?

If it's used to bundle libraries, why is it needed on the server? Why not just bundle everything before deploying or volume the composer directory?

What's your typical use-case @louisremi?

karellm · 2018-03-15T18:46:39Z

@dsifford I'm also fine merging this without it and keep the conversation open. What do you think of the other items? My concern is that this PR will make most PRs non fast-forward.

dsifford · 2018-03-15T19:03:16Z

Yeah, both of the other ones are totally reasonable and I can definitely add those in

dsifford · 2018-03-15T19:07:57Z

@karellm What about instead of hard-coding the reverse proxy stuff, we just add a single EXTRA_PHP env variable that allows users to set whatever extra stuff they might need in the wp-config.php file themselves?

# [...]
environment:
    - EXTRA_PHP: |
        if ($something) {
            // blah blah
        }

karellm · 2018-03-15T19:23:57Z

@dsifford I like that. I think the reverse proxy is also a rare use case. We just need to document this properly.

dsifford · 2018-03-16T21:15:39Z

@karellm Give that a look and let me know what you think. We should be good from here.

One little thing to note, I added a "breaking" change to how URL_REPLACE works. Instead of giving BEFORE_URL,AFTER_URL, now it should only be given AFTER_URL..

If the script comes across the old form, a deprecation warning is printed and it fixes itself. So not technically breaking, but still requires a change to the user's compose file to get rid of the warning.

dsifford · 2018-03-19T17:35:05Z

@karellm updates here?

karellm · 2018-03-19T22:25:23Z

I tried to run it and got:

/run.sh: line 114: mapfile: -d: invalid option
mapfile: usage: mapfile [-n count] [-O origin] [-s count] [-t] [-u fd] [-C callback] [-c quantum] [array]
/run.sh: line 120: plugin_deps[${keyvalue[0]}]: bad array subscript

Does it ring a bell?

Also the README should be updated under "available images" to account for 7.2.

dsifford · 2018-03-19T22:34:17Z

Hmmmmmmm... Where did you run this? In the container?

-d is totally a valid option for mapfile. But it requires a recent version of bash 4, which should be what is being used in the container.

Re: README -- Good catch.

karellm · 2018-03-19T23:11:13Z

I checkout the branch, built the image and referenced it in our company website with a new tag. Is there something more you did?

dsifford · 2018-03-19T23:19:55Z

Which tag did you try?

dsifford · 2018-03-19T23:28:10Z

Hah... Knew it!

The PHP 5.6 image uses bash 4.3, which does not have the -d flag on mapfile.

Uggggh.... Suggestions?

Edit: Nor does the php7.0 image

Edit 2: All images except for the PHP 7.2 image do not have mapfile -d .... I'll refactor that I suppose

dsifford · 2018-03-19T23:54:04Z

@karellm Ok should be good now. Not sure why I even added the -d flag to those two mapfile calls since I specified the delimiter as \n. That's already the default delimiter!

Let me know if you still have issues.

karellm · 2018-03-20T14:45:10Z

Haha, you know the art of making feeling bad for not updating our server's php version ;)

I've just tested the build and the mapfile issue is fixed. Thanks! I encountered two more errors:

Volume mounted themes trigger an error:

volumes:
  - ./wp-content/themes/visible:/app/wp-content/themes/visible
...
THEMES: >-
  visible

Cause:

wordpress_1  | ==> Checking themes
wordpress_1  | Warning: Couldn't find 'visible' in the
wordpress_1  |          WordPress.org theme directory.
wordpress_1  |   Error: No themes installed.

Plugins don't install, I have two errors:

wordpress_1  | Warning: Couldn't find
wordpress_1  |          'advanced-custom-fields-pro' in the WordPress.org plugin
wordpress_1  |          directory.
wordpress_1  |          Activating 'advanced-custom-fields-pro'...
wordpress_1  | Warning: The 'advanced-custom-fields-pro' plugin
wordpress_1  |          could not be found.
wordpress_1  |          Installing Better Font Awesome (1.7.1)
wordpress_1  |          Downloading installation package from
wordpress_1  |          https://downloads.wordpress.org/plugin/better-font-awesome.1.
wordpress_1  |          7.1.zip...
wordpress_1  |          Unpacking the package...
wordpress_1  | Warning: Could not create directory.

Let me know if you need more details. I think the errors above are a little poorly formatted, I guess it is truncated in length but it gets in the way of readability imo, I would rely on the terminal truncation. (update: I can see why it makes copy/pasting log to github issues a lot better though)

dsifford · 2018-03-20T15:49:36Z

@karellm Can you post the snippet to your plugins env variable?

Re: theme issue -- You shouldn't post your volumes in THEMES. I think before we might have guarded for that, but that guard added some extra complexity.

Just the things that need to be downloaded at runtime should go in THEMES and PLUGINS

Re: truncation --- if not truncated manually at 70 chars, then it'll just wrap into the wordpress_1 | line in the terminal and it makes parsing out what's going on sort of difficult IMHO.

karellm · 2018-03-20T16:20:01Z

RE: themes - I forgot about that, we are running an older version 0.15. My bad.
RE: truncation - Works for me
RE: plugins - Here you go:

environment:
  DB_NAME: wordpress
  DB_PASS: root
  WP_DEBUG: 'true'
  THEMES: >-
    visible
  PLUGINS: >-
    add-to-any,
    advanced-custom-fields-pro,
    acf-repeater,
    custom-post-type-ui,
    better-font-awesome,
    disable-visual-editor-wysiwyg,
    formstack,
    gravity-forms-toolbar,
    instapage,
    optimizely,
    mailchimp-for-wp,
    sendgrid-email-delivery-simplified,
    simple-301-redirects,
    swift-mailer,
    sumome,
    unbounce,
    wordpress-importer,
    wordpress-seo,
    wp-maintenance-mode,
    wp-migrate-db,
    wp-video-lightbox

Note that acf-repeater is a local plugin mounted via volume so the same comment as the theme might apply. It will not be available in the wordpress directory

dsifford · 2018-03-20T20:20:35Z

Found a typo in the theme check. I think once I fix that it should work as expected.

Also, this fix should now tolerate plugins and theme volumes being listed explicitly in the compose file.

Let me know.

karellm · 2018-03-21T16:17:06Z

Themes are not failing anymore, it didn't download nothing either. 👍

I still get the unpacking error though:

wordpress_1  | Warning: Could not create directory.
wordpress_1  |          Installing Better Font Awesome (1.7.1)
wordpress_1  |          Downloading installation package from
wordpress_1  |          https://downloads.wordpress.org/plugin/better-font-awesome.1.
wordpress_1  |          7.1.zip...
wordpress_1  |          Unpacking the package...

After checking, it looks like a permission issue. Permission are all weird. Does it ring a bell to you? Removing the mounted volumes didn't help much.

dsifford · 2018-03-21T16:27:15Z

Ahaaaa!

Yeah, forgot that volumes will automatically force root-level perms to the volumed directory.

Sorry for the trouble! And thanks for your continued patience 😄

I'll fix that in a few.

dsifford · 2018-03-27T20:10:11Z

Eeek! Sorry for the delay. Totally forgot about this.

Should be good now @karellm

karellm · 2018-03-28T00:49:52Z

@dsifford I still get the error. Did you test this successfully with local and remote themes/plugins? If not can you please make sure you test all the images? Thanks

dsifford · 2018-03-28T00:50:31Z

Yes I tested with both

karellm · 2018-03-28T16:18:23Z

My bad, the error is different and not an error per say Warning: Plugin 'wp-migrate-db' is already active. I think that the output for plugins could benefit from being cleaned up but that's non-blocking.

Can you juste make sure to squash when you merge this so we keep a clean history?

Thanks for the good work once more!

dsifford · 2018-03-28T18:20:08Z

Ah, yeah -- we used to filter those out but I figured it would be safer and easier to maintain if we just let all messages from wp-cli through.

Re: squash -- yep, no problem will do.

I'll merge this a bit later when I have time to build and push to dockerhub. 👍

Thanks for the great feedback, as always.

dsifford · 2018-04-03T00:57:54Z

Circling back to this now finally @karellm and I think I'm gonna have to add back in the python-certbot-apache dependency to the wordpress image because it becomes too much of a nightmare to renew certificates outside of the wordpress container.

This is because the SSL challenge step of certbot requires connections to port 443, which can't be exposed simultaneously between two running containers.

Is that alright?

karellm · 2018-04-05T13:43:59Z

@dsifford I reviewed the code and it looks ok though I haven't got a chance to test. If you have, I trust you to merge this.

dsifford · 2018-04-07T01:17:07Z

Found a bug... Gonna work on a fix shortly.

This fixes installs in cases where the install source is a url. Previosly, the key was passed, which would result in wp-cli checking wordpress.org for it and it would fail.

dsifford · 2018-04-07T03:21:16Z

Fixed... I'll test again tomorrow

karellm · 2018-04-07T20:39:48Z

Our site seems to run fine and the code looks good to me 👍

dsifford · 2018-04-07T21:02:35Z

Found one more little sneaky one.. I'll push an update for it shortly

Update: Commit below still didn't fix -- working on something better when I get a few extra moments.

various improvements and build updates

ba4f46b

dsifford requested a review from karellm March 14, 2018 14:36

karellm reviewed Mar 14, 2018

View reviewed changes

address code review items

2f56a02

dsifford added 2 commits March 14, 2018 19:23

remove commented out lines

c1ee201

fix copy/paste issue

47b619d

update readme + fix code review issues

da3eec5

dsifford mentioned this pull request Mar 16, 2018

Add user on database test #131

Closed

update README.md

39667b1

fix mapfile issue

acce44d

address code review issues

4e53e6f

fix directory permissions issue when using volumes

399f7f1

karellm approved these changes Mar 28, 2018

View reviewed changes

dsifford added 4 commits April 2, 2018 21:17

add back certbot dependency

a95f8fd

update CHANGELOG

9a528fd

remove old script from package.json

2d0d5a9

fix backports statement in Dockerfile

f44987a

v0.18.0

0e4fde4

fix plugin/theme install issue

436aa2c

This fixes installs in cases where the install source is a url. Previosly, the key was passed, which would result in wp-cli checking wordpress.org for it and it would fail.

dsifford added 2 commits April 7, 2018 12:19

fix default theme fallback logic

aaba095

fix theme/plugin activation issue when using URLs to install

2ec6dfa

dsifford added 2 commits April 7, 2018 17:59

harden permissions

fdb389e

fix permissions

5ca7d11

dsifford merged commit 7227bc5 into master Apr 14, 2018

dsifford mentioned this pull request Apr 14, 2018

Addition to wp-config.php to allow https via reverse proxy #106

Closed

dsifford deleted the dsifford-build-updates branch May 2, 2018 19:56

various improvements and build updates #130

various improvements and build updates #130

Conversation

dsifford commented Mar 12, 2018 • edited Loading

karellm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsifford commented Mar 14, 2018

karellm commented Mar 15, 2018 • edited Loading

dsifford commented Mar 15, 2018

karellm commented Mar 15, 2018

dsifford commented Mar 15, 2018

karellm commented Mar 15, 2018

dsifford commented Mar 15, 2018

dsifford commented Mar 15, 2018

karellm commented Mar 15, 2018

dsifford commented Mar 16, 2018

dsifford commented Mar 19, 2018

karellm commented Mar 19, 2018

dsifford commented Mar 19, 2018

karellm commented Mar 19, 2018

dsifford commented Mar 19, 2018

dsifford commented Mar 19, 2018 • edited Loading

dsifford commented Mar 19, 2018

karellm commented Mar 20, 2018 • edited Loading

Volume mounted themes trigger an error:

Plugins don't install, I have two errors:

dsifford commented Mar 20, 2018 • edited Loading

karellm commented Mar 20, 2018

dsifford commented Mar 20, 2018

karellm commented Mar 21, 2018

dsifford commented Mar 21, 2018

dsifford commented Mar 27, 2018

karellm commented Mar 28, 2018

dsifford commented Mar 28, 2018

karellm commented Mar 28, 2018

dsifford commented Mar 28, 2018

dsifford commented Apr 3, 2018 • edited Loading

karellm commented Apr 5, 2018

dsifford commented Apr 7, 2018

dsifford commented Apr 7, 2018 • edited Loading

karellm commented Apr 7, 2018

dsifford commented Apr 7, 2018 • edited Loading

dsifford commented Mar 12, 2018 •

edited

Loading

karellm commented Mar 15, 2018 •

edited

Loading

dsifford commented Mar 19, 2018 •

edited

Loading

karellm commented Mar 20, 2018 •

edited

Loading

dsifford commented Mar 20, 2018 •

edited

Loading

dsifford commented Apr 3, 2018 •

edited

Loading

dsifford commented Apr 7, 2018 •

edited

Loading

dsifford commented Apr 7, 2018 •

edited

Loading