Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to WordPress 4.7.3 #45

Merged
merged 2 commits into from
Apr 18, 2017
Merged

Update to WordPress 4.7.3 #45

merged 2 commits into from
Apr 18, 2017

Conversation

misterbisson
Copy link
Contributor

@misterbisson misterbisson commented Apr 17, 2017

...and many other updates as needed. See inline comments for more details.

@@ -3,7 +3,8 @@ FROM php:5.6-apache
RUN a2enmod rewrite

# Install the PHP extensions we need, and other packages
RUN apt-get update \
RUN set -ex \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefixing all the run statements with set -ex for easier debugging, as is used in some other repos

&& pecl install memcached \
# Memcached 2.2.0 is the latest for PHP < 7
# see https://pecl.php.net/package/memcached
&& pecl install memcached-2.2.0 \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choosing not to bump the base image up to PHP 7, so I have to be more specific here

&& docker-php-ext-enable memcached \
# Set recommended PHP.ini settings
# See https://secure.php.net/manual/en/opcache.installation.php
&& { \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just joining this with the previous run statement for one less layer

@@ -5,5 +5,5 @@
consul-template \
-once \
-dedup \
-consul ${CONSUL}:8500 \
-consul-addr ${CONSUL}:8500 \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest version of consul-template was throwing warnings/notices about the change to the argument

@@ -34,7 +34,7 @@ consul:
dns:
- 127.0.0.1
labels:
- triton.cns.services=consul
- triton.cns.services=wp-consul
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change for upgrades from previous versions of this project, but we've discovered that namespacing Consul is critical for supporting multiple projects in a single DC

@@ -96,7 +96,7 @@ nginx:
environment:
- CONSUL_AGENT=1
labels:
- triton.cns.services=nginx
- triton.cns.services=wp-nginx, nginx
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I chose to make a breaking change elsewhere, I'm preserving the un-namespaced nginx here because I can't change my Bitly link

@@ -17,6 +17,11 @@
"onChange": "/usr/local/bin/onchange-db.sh"
},
{
"name": "mysql-primary",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have been tracking this all along.

Currently there's a race condition while doing rolling updates of MySQL. If the new mysql-primary becomes healthy after all changes to the mysql services change, then WordPress won't recognize it. This is intended to fix that.

@@ -196,7 +196,7 @@ envcheck() {
echo >> _env

echo '# Consul discovery via Triton CNS' >> _env
echo CONSUL=consul.svc.${TRITON_ACCOUNT}.${TRITON_DC}.cns.joyent.com >> _env
echo CONSUL=wp-consul.svc.${TRITON_ACCOUNT}.${TRITON_DC}.cns.joyent.com >> _env
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applying the new namespacing here

@@ -96,7 +96,7 @@ nginx:
environment:
- CONSUL_AGENT=1
labels:
- triton.cns.services=nginx
- triton.cns.services=wp-nginx,nginx
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I chose to make a breaking change elsewhere, I'm preserving the un-namespaced nginx here because I can't change my Bitly link

@geek
Copy link

geek commented Apr 17, 2017

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants