Large refactor + improvements #97

dsifford · 2017-04-23T21:24:51Z

@karellm This PR addresses a bunch of the nagging issues that have been around for a while now. There's a lot of changes, all good I think, so, in an effort to reduce verbosity, I'll copy the changelog verbatim below...

0.15.0 - latest

Deprecations

Note: Both deprecations will still work as usual until the next release cycle.

`[local]plugin-name` & `[local]theme-name` syntax

The build process is now aware of locally-volumed plugins and themes automatically.

Additionally, listing locally-volumed plugins and themes in your docker-compose.yml file is optional; you may list them if you'd like to keep your compose file declarative, or you may skip listing them completely. The build will complete the same either way.

`SEARCH_REPLACE` has been renamed to `URL_REPLACE`

We chose to rename this because, although you may search and replace strings that are not URLs, the build process requires them to be.

This name reflects that requirement better and will lead to less confusion down the road.

`VERBOSE` environment variable

Logging has been changed to show necessary information by default.

Improvements

Widespread efficiency improvements to build process.
Reduce the number of Dockerfile layers.
If SERVER_NAME is specified (eg. example.com), create a ServerAlias in the apache configs for www.example.com.

Fixes

Plugins and themes are only pruned if they meet the following criteria (Closes Pruning themes and plugins is overzealous #51):
- Not listed in docker-compose.yml.
- Not added as a local volume.
Adjust permissions of volumed files and directories so that they remain editable outside the container, and remain secure. (Closes wp-content folder permission issues #12)

Final Note: run.sh was pretty much rewritten again because it was sort of a disaster, which is my fault. A lot of the complexity from the last version came from just making the logging pretty. I changed a bunch of that, which I'm sure you'll see. I was able to slim down the file a good bit and keep it readable.

Let me know your thoughts! Glad to discuss anything that you aren't sure of or think can be done better.

dsifford · 2017-04-27T22:02:23Z

Have you had a chance to look through this @karellm? Any questions?

dsifford · 2017-04-29T20:03:27Z

@karellm Merging this monday if I don't hear back. Need the fixes on my sites.

karellm · 2017-04-30T23:45:27Z

@dsifford I disagree about merging code that wasn't reviewed. I will take the time tomorrow to look at it (past few weeks have been quite busy for me).

dsifford · 2017-05-01T00:09:29Z

@karellm No problem. Take all the time you need. I just didn't hear back with even an acknowledgment of the PR so that's why I was opting to merge if I didn't hear back. I didn't know what your particular circumstances were or if you'd be able to review in a semi-timely fashion.

karellm

Great work, the project is a lot cleaner now. I added couple minor comments. I will also test it with a website tomorrow.

karellm · 2017-05-02T02:21:55Z

README.md


 # start the website at localhost:8080
-docker-compose up
+docker-compose up -d && docker-compose logs -f wordpress


What is the motivation behind this?

That's how I start it. It allows you to watch build logs and know when exactly the build is finished, rather than just trying localhost:8080 a bunch of times until it works.

I haven't run a wordpress site in a while but I think docker-compose up did output the logs for me. It is arguably more verbose though since it logs all services and not just wordpress.

Yeah, the benefit to doing my way is that you get only the wordpress logs, as you stated, and also you can just ctrl-c after it's done building without it tearing down the application.

You can detach from a forground process by hitting ctrl-p followed by ctrl-q as well (I think), but that's too difficult to remember.

karellm · 2017-05-02T02:23:36Z

README.md

- [Part 3: Optimize your wordpress theme assets and deploy to S3](https://visible.vc/engineering/optimize-wordpress-theme-assets-and-deploy-to-s3-cloudfront/)
- Part 4: Auto deploy your site on your server (coming)
-
-### Example


The Example section was removed altogether. Why is that?

I moved it to its own ./example directory to contain all of it there for those who need a complete example to look at.

My question wasn't clear. I think that there is value in the documentation. You might want to use this without relying on the example at all no?

What about moving the example details to a separate README.md file in the example directory? That way the info is still there for those who need it, but it clears up some of the noise from the main readme.

I'm fine with that

karellm · 2017-05-02T02:30:16Z

package.json

-      "accessKeyId": "",
-      "secretAccessKey": ""
-    }
+    "update-version": "for file in './php7.0/Dockerfile' './php5.6/Dockerfile'; do sed -Ei \"s/(version=\\\")(.*)(-)/\\1$npm_package_version\\3/g\" \"$file\"; done && sed -Ei \"s/(wordpress:)(.*)(-)/\\1$npm_package_version\\3/g\" ./example/docker-compose.yml"


🎉 I think you missed 7.1 though

Yep, good catch. Adding 7.1 was an afterthought. I'll fix later this afternoon.

karellm · 2017-05-02T02:32:03Z

php5.6/Dockerfile

-# Set recommended PHP.ini settings (see https://secure.php.net/manual/en/opcache.installation.php)
-RUN echo "memory_limit = 512M" > /usr/local/etc/php/php.ini \
+        zip \
+    # See https://secure.php.net/manual/en/opcache.installation.php)


Trailing ) not needed

karellm · 2017-05-02T02:32:14Z

php7.0/Dockerfile

-# Set recommended PHP.ini settings (see https://secure.php.net/manual/en/opcache.installation.php)
-RUN echo "memory_limit = 512M" > /usr/local/etc/php/php.ini \
+        zip \
+    # See https://secure.php.net/manual/en/opcache.installation.php)


karellm · 2017-05-02T02:34:00Z

php7.1/Dockerfile

+        opcache \
+        soap \
+        zip \
+    # See https://secure.php.net/manual/en/opcache.installation.php)


karellm · 2017-05-02T02:42:50Z

run.sh

-  h1 "WordPress Configuration Complete!"
+  while read -r -d, i; do
+    [[ ! "$i" ]] && continue
+    i="${i# }"          # Trim leading whitespace


Could maybe be a function?

I suppose it could. No preference. I can change later this afternoon.

Actually, I take that back. It would end up being more code than just keeping as is because bash functions can't return values. So there'd have to be a minimum of 2 side effects each time it's called, which I'm not super fond of.

I can still change it if you're firm on this, but I wouldn't recommend it.

That's fine as is!

dsifford · 2017-05-02T19:04:24Z

@karellm I addressed everything you mentioned in the last commit, less the part about converting the string replacements into a function. Gonna wait to hear back on that before I travel that road.

Let me know what else you need.

karellm · 2017-05-03T13:41:10Z

I still need to run it but looks good. I will do that later today

dsifford · 2017-05-06T18:08:56Z

You get a chance to run this yet? Any issues?

dsifford · 2017-05-12T23:55:37Z

@karellm status?

dsifford · 2017-05-22T00:57:55Z

@karellm Can you please let me know if I can merge this? I really need the update.

Thanks in advance.

karellm · 2017-05-23T15:30:40Z

@dsifford I apologize for the delay. I was offline for a while (a much needed break).

I just ran everything and it went smoothly. Also did another pass of the code and didn't see anything bugging me. So let's merge this. Thanks again for the great work!

dsifford · 2017-05-23T16:06:16Z

No problem. Everyone needs a good disconnect every now and again! Glad to hear yours went well. 😄

Gonna merge and let my automatic builds run. If you'd like to also update your dockerhub builds, can you take care of those?

Thanks!

karellm · 2017-05-23T17:08:38Z

@dsifford I looked and the Hub still require us to grant full access to all private repos, which I don't want to do. That said, I simplified the way to deploy a bit: https://github.com/visiblevc/wordpress-starter/wiki/Development

dsifford · 2017-05-23T17:10:21Z

@karellm I have automated builds pulling from this repo here: https://hub.docker.com/r/dsifford/wordpress/builds/

On a different note: found a couple issues with the run.sh file.. Patch coming shortly

big refactor + improvements/bugfixes

00f2d8d

dsifford requested a review from karellm April 23, 2017 21:25

dsifford added 2 commits April 24, 2017 12:46

add security fix from #98

8f5c3ae

fix rewrite flush

399de99

Add php7.1 dockerfile

01c9667

adjust volume check from grout=1000 to group!=root for better coverage

4568387

dsifford mentioned this pull request May 1, 2017

wp-content folder permission issues #12

Closed

karellm reviewed May 2, 2017

View reviewed changes

address items from code review

4a50d3d

dsifford merged commit 76150f8 into master May 23, 2017

dsifford deleted the dsifford-refactor-fixes branch May 23, 2017 16:06

dsifford mentioned this pull request May 27, 2017

invalid image version in example docker-compose #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Large refactor + improvements #97

Large refactor + improvements #97

dsifford commented Apr 23, 2017 •

edited

dsifford commented Apr 27, 2017

dsifford commented Apr 29, 2017

karellm commented Apr 30, 2017

dsifford commented May 1, 2017

karellm left a comment

karellm May 2, 2017

dsifford May 2, 2017

karellm May 2, 2017

dsifford May 2, 2017

karellm May 2, 2017

dsifford May 2, 2017 •

edited

karellm May 2, 2017

dsifford May 2, 2017

karellm May 3, 2017

karellm May 2, 2017

dsifford May 2, 2017

karellm May 2, 2017

karellm May 2, 2017

karellm May 2, 2017

karellm May 2, 2017

dsifford May 2, 2017

dsifford May 2, 2017

karellm May 3, 2017

dsifford commented May 2, 2017

karellm commented May 3, 2017

dsifford commented May 6, 2017

dsifford commented May 12, 2017

dsifford commented May 22, 2017

karellm commented May 23, 2017

dsifford commented May 23, 2017

karellm commented May 23, 2017

dsifford commented May 23, 2017

Large refactor + improvements #97

Large refactor + improvements #97

Conversation

dsifford commented Apr 23, 2017 • edited

0.15.0 - latest

Deprecations

[local]plugin-name & [local]theme-name syntax

SEARCH_REPLACE has been renamed to URL_REPLACE

VERBOSE environment variable

Improvements

Fixes

dsifford commented Apr 27, 2017

dsifford commented Apr 29, 2017

karellm commented Apr 30, 2017

dsifford commented May 1, 2017

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

dsifford May 2, 2017 • edited

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 May 2, 2017

karellm commented May 3, 2017

dsifford commented May 6, 2017

dsifford commented May 12, 2017

dsifford commented May 22, 2017

karellm commented May 23, 2017

dsifford commented May 23, 2017

karellm commented May 23, 2017

dsifford commented May 23, 2017

dsifford commented Apr 23, 2017 •

edited

`[local]plugin-name` & `[local]theme-name` syntax

`SEARCH_REPLACE` has been renamed to `URL_REPLACE`

`VERBOSE` environment variable

dsifford May 2, 2017 •

edited