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

Release 0.2.0 #55

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Release 0.2.0 #55

wants to merge 8 commits into from

Conversation

petewilcock
Copy link
Contributor

@petewilcock petewilcock commented Apr 5, 2022

Resolves #54
Resolves #48
Resolves #24
Resolves #57
Resolves #50

@nvnivs
Copy link
Collaborator

nvnivs commented Apr 29, 2022

This also resolves #50

One question, are you able now to run the workflows against this PR? Curious to see if my PRs broke any of the inspections.

@petewilcock
Copy link
Contributor Author

petewilcock commented Apr 29, 2022

are you able now to run the workflows against this PR? Curious to see if my PRs broke any of the inspections.

Not on the fork. I tried upgrading the tests but it still seems to have the same issue with refspec. I'd need more time to dig into it. Once your PRs get merged to 0.2.0 the tests are run again, and the pre-commit config will make an auto-commit (as you can see here) to fix things like regeneration of terraform-docs. The only other failing test is yamllint complaining about the linelength of the buildspec of codebuild. I've actually got a comment which should ignore that specific test, but it doesn't seem to be honouring it for some reason.

If you install pre-commit yourself, the tests will also run locally before it allows you to complete a commit. Failing tests will auto-fix in some cases, and then you can commit again.

@z0c I'll solve the problem by adding you as a contributor, then you can branch directly on this repo.

* Passes cloudfront_function_301_redirects to cloudfront module

* Fixes missing uri param

* Passes plugin version from top level module

* Updates WP2Static url as repo was migrated

* Adds some help of permanent redirects

* Removed bad paste
@nvnivs
Copy link
Collaborator

nvnivs commented Apr 29, 2022

Thanks @petewilcock . Actions can be a pain with forks, sometimes a workaround for this kind of issues is to run on the pull_request_target, but that is full of security implications as well...

Next change i will branch in this repo 👍


cloudfront_class = var.cloudfront_class
waf_acl_arn = var.waf_enabled ? module.waf[0].waf_acl_arn : null
cloudfront_function_301_redirects = var.cloudfront_function_301_redirects
Copy link

Choose a reason for hiding this comment

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

Suggested change
cloudfront_function_301_redirects = var.cloudfront_function_301_redirects
cloudfront_function_301_redirects = var.cloudfront_function_301_redirects
site_prefix = var.site_prefix

See #66

Comment on lines +4 to +5
ARG wp2static_version
ARG wp2static_s3_addon_version
Copy link

@blimmer blimmer May 15, 2022

Choose a reason for hiding this comment

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

Each time you use a FROM, it creates a new docker layer and these args are lost.

So, you need to pull these two ARG declarations down below the FROM for it to work properly. See https://benkyriakou.com/posts/docker-args-empty for more info.

Otherwise, the arguments will be empty when you try to use them below.

Comment on lines +13 to +14
RUN curl https://github.com/WP2Static/wp2static/archive/refs/tags/${wp2static_version}.zip -o /tmp/serverless-wordpress-wp2static.zip
RUN curl https://github.com/leonstafford/wp2static-addon-s3/archive/refs/tags/${wp2static_s3_addon_version}.zip -o /tmp/serverless-wordpress-s3-addon.zip
Copy link

Choose a reason for hiding this comment

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

Suggested change
RUN curl https://github.com/WP2Static/wp2static/archive/refs/tags/${wp2static_version}.zip -o /tmp/serverless-wordpress-wp2static.zip
RUN curl https://github.com/leonstafford/wp2static-addon-s3/archive/refs/tags/${wp2static_s3_addon_version}.zip -o /tmp/serverless-wordpress-s3-addon.zip
RUN curl -Lf https://github.com/WP2Static/wp2static/archive/refs/tags/${wp2static_version}.zip -o /tmp/serverless-wordpress-wp2static.zip
RUN curl -Lf https://github.com/leonstafford/wp2static-addon-s3/archive/refs/tags/${wp2static_s3_addon_version}.zip -o /tmp/serverless-wordpress-s3-addon.zip

In addition to the comment about the ARGs needing to move down (above), these commands were silently failing.

 /tmp  curl https://github.com/WP2Static/wp2static/archive/refs/tags/7.1.7.zip -o /tmp/serverless-wordpress-wp2static.zip
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
 /tmp  ls -alh /tmp/serverless-wordpress-wp2static.zip
-rw-r--r-- 1 blimmer wheel 0 May 14 20:59 /tmp/serverless-wordpress-wp2static.zip

You need to pass -L to follow redirects for this to work:

 /tmp  curl -L https://github.com/WP2Static/wp2static/archive/refs/tags/7.1.7.zip -o /tmp/serverless-wordpress-wp2static.zip
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  153k    0  153k    0     0   206k      0 --:--:-- --:--:-- --:--:--     0
 /tmp  ls -alh /tmp/serverless-wordpress-wp2static.zip
-rw-r--r-- 1 blimmer wheel 154K May 14 21:00 /tmp/serverless-wordpress-wp2static.zip

Also, you should pass -f so curl will fail if it encounters a non-20x HTTP response code:

 /tmp  curl -Lf https://github.com/WP2Static/wp2static/archive/refs/tags/not-a-real-release.zip -o /tmp/serverless-wordpress-wp2static.zip
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (22) The requested URL returned error: 404
 ✘  /tmp  echo $?
22

COPY docker-entrypoint.sh /usr/local/bin/
RUN apt-get update && apt-get install -y sudo jq awscli mariadb-client && chmod +x /usr/local/bin/docker-entrypoint.sh && chmod +x /tmp/wp-cli.phar && mv /tmp/wp-cli.phar /usr/local/bin/wp \
&& rm -rf /var/lib/apt/lists/*

RUN curl https://github.com/WP2Static/wp2static/archive/refs/tags/${wp2static_version}.zip -o /tmp/serverless-wordpress-wp2static.zip
Copy link

Choose a reason for hiding this comment

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

I'm seeing some issues with using this source code directly:

Screen Shot 2022-05-14 at 21 06 11

According to the docs they referenced in that message, it sounds like we need to do some additional work with this source code to make it work.

Copy link

Choose a reason for hiding this comment

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

Alright, I got wp2static working with a mutli-stage build:

ARG aws_account_id
ARG aws_region
ARG ecr_repo_name

FROM php:7.4 as compile

ARG wp2static_version

WORKDIR /deps
RUN apt-get update && \
    apt-get install --no-install-recommends -y wget zip unzip && \
    rm -fr /var/lib/apt/lists/*
RUN wget https://raw.githubusercontent.com/composer/getcomposer.org/main/web/installer -O - -q | php -- --quiet && \
    mv /deps/composer.phar /usr/local/bin/composer
RUN curl -Lf https://github.com/WP2Static/wp2static/archive/refs/tags/${wp2static_version}.zip -o /deps/serverless-wordpress-wp2static.zip && \
    unzip /deps/serverless-wordpress-wp2static.zip && \
    cd /deps/wp2static-${wp2static_version} && \
    composer install && \
    composer build serverless-wordpress-wp2static && \
    mv $HOME/Downloads/serverless-wordpress-wp2static.zip /deps/serverless-wordpress-wp2static.zip

ARG aws_account_id
ARG aws_region
ARG ecr_repo_name

FROM ${aws_account_id}.dkr.ecr.${aws_region}.amazonaws.com/${ecr_repo_name}:base as wordpress

ARG wp2static_s3_addon_version

COPY ["wp-cli.phar", "/tmp/"]
COPY docker-entrypoint.sh /usr/local/bin/
RUN apt-get update && apt-get install -y sudo jq awscli mariadb-client && chmod +x /usr/local/bin/docker-entrypoint.sh && chmod +x /tmp/wp-cli.phar && mv /tmp/wp-cli.phar /usr/local/bin/wp \
&& rm -rf /var/lib/apt/lists/*

COPY --from=compile /deps/serverless-wordpress-wp2static.zip /tmp/serverless-wordpress-wp2static.zip
RUN curl -Lf https://github.com/leonstafford/wp2static-addon-s3/archive/refs/tags/${wp2static_s3_addon_version}.zip -o /tmp/serverless-wordpress-s3-addon.zip

RUN mv "$PHP_INI_DIR/php.ini-production" "$PHP_INI_DIR/php.ini"
COPY ["php.ini", "$PHP_INI_DIR/conf.d/"]

Now that plugin is passing, but the other plugin isn't working still. I'm going to call it a night and I'll see if I can get the other one working. It feels like I'm close.

Screen_Shot_2022-05-14_at_21_56_13

Copy link
Contributor Author

Choose a reason for hiding this comment

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

many thanks @blimmer - some thoughtful and useful comments here. I now remember another reason why I'd opted to bundle the pre-compiled of WP2Static - because the releases are source code only! I think the same is true for the S3 add-on.

I think we'll also need to be careful to remove any source files from the layer to avoid bloating out the image.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for spotting this @blimmer! These are very good points, but im quite baffled as to why my dev instance is working fine... Clearly missed something, need a closer inspection.

Copy link

Choose a reason for hiding this comment

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

I think we'll also need to be careful to remove any source files from the layer to avoid bloating out the image.

@petewilcock - if you mean you want to make sure all the composer dependencies are removed from the final build, this is exactly why I'm using a multi-stage build here 😄 TLDR; the first "FROM" is a layer has all the dev dependencies and we copy the single built zip file over to the final layer. This helps keep the final docker image small

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @blimmer I wasn't paying enough attention to the diff, looks good :)

Copy link

Choose a reason for hiding this comment

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

I just got this working in my account with this dockerfile:

ARG aws_account_id
ARG aws_region
ARG ecr_repo_name

# This stage is used to install plugin dependencies using composer. We use a docker multi-stage build to ensure all
# these extra dependencies do not end up in the final layer.
FROM php:7.4 as compile

ARG wp2static_version
ARG wp2static_s3_addon_version

WORKDIR /deps

# Install system dependencies
RUN apt-get update && \
    apt-get install --no-install-recommends -y wget zip unzip && \
    rm -fr /var/lib/apt/lists/*

# Install composer
RUN wget https://raw.githubusercontent.com/composer/getcomposer.org/main/web/installer -O - -q | php -- --quiet && \
    mv /deps/composer.phar /usr/local/bin/composer

# Download wp2static source code, install composer dependencies and build production plugin bundle
RUN curl -Lf https://github.com/WP2Static/wp2static/archive/refs/tags/${wp2static_version}.zip -o /deps/serverless-wordpress-wp2static.zip && \
    unzip /deps/serverless-wordpress-wp2static.zip && \
    cd /deps/wp2static-${wp2static_version} && \
    composer install && \
    composer build serverless-wordpress-wp2static && \
    mv $HOME/Downloads/serverless-wordpress-wp2static.zip /deps/serverless-wordpress-wp2static.zip

# Download wp2static-s3-addon source code, install composer dependencies and build production plugin bundle
RUN curl -Lf https://github.com/leonstafford/wp2static-addon-s3/archive/refs/tags/${wp2static_s3_addon_version}.zip -o /deps/serverless-wordpress-s3-addon.zip && \
    unzip /deps/serverless-wordpress-s3-addon.zip && \
    cd /deps/wp2static-addon-s3-${wp2static_s3_addon_version} && \
    composer install && \
    composer build serverless-wordpress-s3-addon && \
    mv $HOME/Downloads/serverless-wordpress-s3-addon.zip /deps/serverless-wordpress-s3-addon.zip

# Start over with a fresh wordpress layer
ARG aws_account_id
ARG aws_region
ARG ecr_repo_name
FROM ${aws_account_id}.dkr.ecr.${aws_region}.amazonaws.com/${ecr_repo_name}:base as wordpress

COPY ["wp-cli.phar", "/tmp/"]
COPY docker-entrypoint.sh /usr/local/bin/
RUN apt-get update && apt-get install -y sudo jq awscli mariadb-client && chmod +x /usr/local/bin/docker-entrypoint.sh && chmod +x /tmp/wp-cli.phar && mv /tmp/wp-cli.phar /usr/local/bin/wp \
&& rm -rf /var/lib/apt/lists/*

# Copy over built plugins for the compile layer
COPY --from=compile /deps/serverless-wordpress-wp2static.zip /tmp/serverless-wordpress-wp2static.zip
COPY --from=compile /deps/serverless-wordpress-s3-addon.zip /tmp/serverless-wordpress-s3-addon.zip

RUN mv "$PHP_INI_DIR/php.ini-production" "$PHP_INI_DIR/php.ini"
COPY ["php.ini", "$PHP_INI_DIR/conf.d/"]

The only outstanding thing you might want to think about is the PHP layer version. You might want to make that configurable in case someone is using a different version of PHP. I'm not familiar at all with the PHP ecosystem, so not sure how important matching versions is.

@@ -34,6 +35,13 @@
"readOnly": false
}
],
"healthCheck": {
"retries": 10,
"command": [ "CMD-SHELL", "curl -f http://localhost:80/ || exit 1" ],
Copy link
Collaborator

Choose a reason for hiding this comment

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

@petewilcock something i came across using this branch;

If WP goes into maintenance mode (like when upgrading some plugins) the healthcheck will fai terminating the container.

This can leave the file system and/or db in a bad state. I did #65 while looking into this, had to exec to the container to manually get my dev instance back to a working state.

Going forward, the PR above also adds a var to toggle the healthcheck, which at least give a process to do maintenance by disabling it. Not great user experience though.

But think the healthcheck needs a bit more investigation, i've seen online some recommending /wp-admin/install.php or /wp-admin/images/wordpress-logo.svg as better healthcheck, but haven't had a chance to try because had no maintenance updates so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I had no idea the check could potentially fail in maintenance mode. Still not sure why it would. Even in maintenance mode, does the container not respond on port 80? Or is it getting a 301 or something that's throwing it off? Interesting...

Copy link
Collaborator

Choose a reason for hiding this comment

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

It returns a 503, with an actual body of this site is under maintenance, wait a bit... kind of message.

This is the post were I saw the recommendation to /wp-admin/install.php as a workaround bitnami/charts#3019

I haven't been able to test it myself yet.

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