various improvements and build updates

[dsifford]

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 #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:

• [x] Remove commented out lines in Dockerfile
• [x] Update changelog
• [x] Version bump
• [x] Build and push to Dockerhub

Let me know what you think @karellm

[dsifford]

@karellm should be good now

[karellm]

@dsifford Would you mind including these:

• composer (close #76)
• reverse proxy (close #106)
• support for locale (close #124)

[dsifford]

@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]

@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]

@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]

@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]

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

[dsifford]

@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]

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

[dsifford]

@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]

@karellm updates here?

[karellm]

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]

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]

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]

Which tag did you try?

[dsifford]

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]

@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]

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]

@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]

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]

Re: ACF Pro -- that's likely because it's a volumed resource. Correct?

Re: Better font awesome. Not sure. I'll have to check on that.

If you remove all the volumed plugins from the list, does it work then?

A workaround that we might be able to add in is to just check wp plugin is-installed prior to installing.

[karellm]

@dsifford Removing the mounted plugins doesn't fix the other issue Warning: Could not create directory. when a package is getting unpacked.

Also, removing the theme cause this:

wordpress_1  | ==> Checking themes
wordpress_1  | Warning: Can't delete the currently active theme:
wordpress_1  |          visible
wordpress_1  |   Error: No themes deleted.
wordpress_1  |          Installing Twenty Seventeen (1.4)
wordpress_1  |          Downloading installation package from
wordpress_1  |          https://downloads.wordpress.org/theme/twentyseventeen.1.4.zip
wordpress_1  |          ...
wordpress_1  |          Unpacking the package...
wordpress_1  | Warning: Could not create directory.
wordpress_1  |   Error: No themes installed.

Which I'm not huge on. First I don't want the extra theme. Second, I think it make a lot more sense to list all plugins/themes and activating them if they are mounted. wdyt?

[dsifford]

Yeah, clearly there still needs to be a few more guards added.

I'll work on those and chime back when it's done.

TBH, I'm not even sure what could be causing the "cannot create directory" error. I've never seen that one before.

[dsifford]

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]

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]

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 :smile:

I'll fix that in a few.

[dsifford]

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

Should be good now @karellm

[karellm]

@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]

Yes I tested with both

[karellm]

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]

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. :+1:

Thanks for the great feedback, as always.

[dsifford]

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]

@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]

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

[dsifford]

Fixed... I'll test again tomorrow

[karellm]

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

[dsifford]

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.