Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .buildkite/hooks/post-command
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#!/bin/bash

# Only run this on AWS agents
if [[ -z "${BUILDKITE_AGENT_META_DATA_AWS_REGION}" ]]; then
exit 0
fi

mkdir -p logs

echo "--- :docker: Saving Docker Logs"
docker-compose logs wpcli > logs/wpcli.log
docker-compose logs wordpress > logs/wordpress.log
docker-compose logs mysql > logs/mysql.log

buildkite-agent artifact upload "logs/*.log"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this hook was executed twice on VM and host. It also runs on some steps that don't use docker. That results in duplicated empty artifacts:

Screenshot 2024-03-08 at 9 57 47 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in cba14ff, thanks!!

79 changes: 79 additions & 0 deletions .buildkite/pipeline.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# Nodes with values to reuse in the pipeline.
common_params:
plugins: &common_plugins
- automattic/a8c-ci-toolkit#2.17.0
# Common environment values to use with the `env` key.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth adding the default agent here for accessibility.

steps:
#
# Rust Group
- group: ":rust: Core Library"
key: "rust"
steps:
- label: ":rust: Build and Test"
command: |
echo "--- :rust: Building + Testing"
make test-rust
- label: ":rust: Lint"
command: |
echo "--- :rust: Running Clippy"
make lint-rust
#
# Swift Group
- group: ":swift: Swift Wrapper"
key: "swift"
steps:
- label: ":swift: Build and Test"
command: |
echo "--- :rust: Installing Rust"
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -v -y

source "/Users/builder/.cargo/env"

echo "--- :package: Installing Rust Toolchains"
rustup target add x86_64-apple-ios
rustup target add aarch64-apple-ios
rustup target add aarch64-apple-darwin
rustup target add x86_64-apple-darwin
rustup target add aarch64-apple-ios-sim

rustup toolchain install nightly
rustup component add rust-src --toolchain nightly-aarch64-apple-darwin

echo "--- :swift: Building + Testing"
make test-swift
env:
IMAGE_ID: xcode-15.2
plugins: *common_plugins
agents:
queue: mac
- label: ":swift: Lint"
command: |
echo "--- :swift: Swiftlint"
make lint-swift
#
# Docker Group
- group: ":wordpress: End-to-end Tests"
key: "e2e"
steps:
- label: ":wordpress: WordPress {{matrix}}"
command: |
echo "--- :docker: Setting up Test Server"
make test-server

echo "--- 🧪 Running Tests"
curl -u test@example.com:$(cat test_credentials) 'http://localhost/wp-json/wp/v2/posts?context=edit' --fail-with-body | jq
env:
WORDPRESS_VERSION: "{{matrix}}"
matrix:
- '6.4'
- '6.3'
- '6.2'
- '6.1'
- '6.0'
- '5.9'
- '5.8'
- '5.7'
- '5.6' # First version to introduce appliation passwords


38 changes: 38 additions & 0 deletions .buildkite/setup-test-site.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is used in Docker compose, it probably doesn't belong to the .buildkite directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a pattern taken from elsewhere – scripts and CI-related stuff can go in here.

I'm fine to leave it a bit awkward until/unless we make a scripts directory or something, but I figure for just this it might not make sense to clutter up the root directory?

I assume we'll have more of these before long!

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of using scripts directory for this kind of stuff. This repo will likely have several scripts that'll be used outside of the CI, so I don't think we need to wait to have more scripts for this.

Even with a single script, I encountered this same issue before and ended up adding a scripts directory in WCAndroid. (which probably is not necessary anymore with the Releases v2 🤔 )


set -e

# This script sets up a WordPress test site on the `wordpress` docker image.
# You might wonder "why not do this work once, then just import the database for each run?"
# We do each step each time for each build because we're trying to get a "mint" condition site
# for each WordPress version – if there are issues with DB migrations, different default themes
# available, etc we don't want to have to deal with them.

## Install WordPress
wp core download --force

wp core install \
--url=localhost \
--title=my-test-site \
--admin_user=test@example.com \
--admin_email=test@example.com \
--admin_password=strongpassword \
--skip-email

## Ensure URLs work as expected
wp rewrite structure '/%year%/%monthnum%/%postname%/'
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just curious about this one. Does this have any risk of our setup being different from other sites - making the tests less useful?

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 the closest thing to a "default" that the ecosystem has IMHO.

Fortunately, it's easy to change later without breaking anything else


## Download the sample data (https://codex.wordpress.org/Theme_Unit_Test)
curl https://raw.githubusercontent.com/WPTT/theme-unit-test/master/themeunittestdata.wordpress.xml -C - -o /tmp/testdata.xml

## Then install the importer plugin
wp plugin install wordpress-importer --activate

## Then install the test data (https://developer.wordpress.org/cli/commands/import/)
wp import /tmp/testdata.xml --authors=create

## Then clean up the importer plugin
wp plugin delete wordpress-importer

## Create an Application password for the admin user, and store it where it can be used by the test suite
wp user application-password create test@example.com test --porcelain >> /tmp/test_credentials
7 changes: 6 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,10 @@ local.properties
# Auto-generated Swift Files
native/swift/Sources/wordpress-api-wrapper/*.swift

# Temporary test credentials
# Test Server
.wordpress
test_credentials
/logs

# CI Cache
Copy link
Contributor

Choose a reason for hiding this comment

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

I left a comment about CARGO_HOME in Makefile, but if we do decide to keep the .cargo folder in the project, I think we should mention that this is about Docker and not CI since we can and probably will run Docker commands from local environment as well.

.cargo
6 changes: 6 additions & 0 deletions .swiftlint.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
swiftlint_version: 0.53.0
excluded: # paths to ignore during linting. Takes precedence over `included`.
- .cargo # Local Cargo cache
- .build # Compiled Code
- native/swift/Sources/wordpress-api-wrapper/wp_api.swift # auto-generated code
- target # Compiled Code
30 changes: 30 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ udl_path := wp_api/src/wp_api.udl
docker_container_repo_dir=/app

# Common docker options
rust_docker_container := public.ecr.aws/docker/library/rust:1.76
swiftlint_container := ghcr.io/realm/swiftlint:0.53.0

docker_opts_shared := --rm -v "$(PWD)":$(docker_container_repo_dir) -w $(docker_container_repo_dir)
rust_docker_run := docker run -v $(PWD):/$(docker_container_repo_dir) -w $(docker_container_repo_dir) -it -e CARGO_HOME=/app/.cargo $(rust_docker_container)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious about the location choice for the CARGO_HOME.

I got very confused about this when I was reviewing the change about it in .gitignore, because there shouldn't be a .cargo folder in the project in a regular Rust project setup.

Do we need the CARGO_HOME in Docker to be in the /app directory? If not, wouldn't it be easier to not put it there and then we won't have to ignore it in other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a pragmatism vs correctness question here 😅

By default, the Cargo cache isn't persisted between runs so a ton of time is wasted re-fetching the dependency information every time you do anything. There are two ways to cache it and fix this:

  1. Mount an external cache into the container – maybe sharing the same one that the host uses. You can have weirdness with that on Linux if you're not careful about file permissions – Docker can create a directory that the host can't modify later. Also, from a security/safety perspective I like to avoid Docker mounting anything outside the project directory.
  2. Mount the project directory, and have everything in the Docker container work relative to that – this is why there's a .cargo directory in a seemingly odd place. By putting it there, it's persisted between Docker invocations but there's no possibility of it messing up the developer's workstation because it's an isolated cache.

We could rename it to .cache or even put it under target – doesn't really matter where it goes, but it's much faster to run code under Docker if we cache the results this way.

Copy link
Contributor

@oguzkocer oguzkocer Mar 6, 2024

Choose a reason for hiding this comment

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

Thank you for the explanation. Now that I understand why we are doing this, I think the main issue is that it's called .cargo because it's the default name and can get mixed up with the actual CARGO_HOME the host uses. If we rename it to something like .docker_cargo_home, then there are no issues. With a specific name, it's also fairly easy to figure out how it's used by searching the name in the repo. What do you think?

P.S: I actually spent some time researching the .cargo directory while reviewing the PR. It took me embarrassingly long to realize that it could be something we are generating. 😞

docker_build_and_run := docker build -t foo . && docker run $(docker_opts_shared) -it foo

clean:
Expand Down Expand Up @@ -147,6 +151,32 @@ test-android: bindings _test-android

publish-android-local: bindings _publish-android-local

test-rust:
$(rust_docker_run) cargo test

test-server:
rm -rf test_credentials && touch test_credentials && chmod 777 test_credentials
docker-compose up -d
docker-compose run wpcli

stop-server:
docker-compose down

lint: lint-rust lint-swift

lint-rust:
$(rust_docker_run) /bin/bash -c "rustup component add clippy && cargo clippy --all -- -D warnings"

lint-swift:
docker run -v $(PWD):$(docker_container_repo_dir) -w $(docker_container_repo_dir) -it $(swiftlint_container) swiftlint

lintfix-swift:
docker run -v $(PWD):$(docker_container_repo_dir) -w $(docker_container_repo_dir) -it $(swiftlint_container) swiftlint --autocorrect

build-in-docker:
$(call bindings)
$(docker_build_and_run)

dev-server:
mkdir -p .wordpress
docker-compose up
2 changes: 1 addition & 1 deletion Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ let package = Package(
.target(
name: "wordpress-api",
dependencies: [
.target(name: "wordpress-api-wrapper"),
.target(name: "wordpress-api-wrapper")
],
path: "native/swift/Sources/wordpress-api"
),
Expand Down
61 changes: 61 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
version: '3'
services:
wordpress:
image: 'public.ecr.aws/docker/library/wordpress:${WORDPRESS_VERSION:-latest}'
ports:
- '80:80'
volumes:
- .wordpress:/var/www/html
environment:
WORDPRESS_DB_HOST: mysql
WORDPRESS_DB_USER: wordpress
WORDPRESS_DB_PASSWORD: wordpress
WORDPRESS_DB_NAME: wordpress
WORDPRESS_CONFIG_EXTRA: |
# Allow application passwords to be used without HTTPS
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, is that because we'd have certificate issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For security, if a WordPress site isn't using HTTPS application passwords are disabled entirely, so this configuration item says "it's okay, this is a local installation" which gives us more debug info on errors and re-enables application passwords – seems like a nice win all around

Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason I thought even the local installation would use a debug certificate and use HTTPS rather than using HTTP. Thank you for clarifying.

"it's okay, this is a local installation" which gives us more debug info on errors

I think this is enough to mark this as local. My question was more about the comment about HTTPS.

define( 'WP_ENVIRONMENT_TYPE', 'local' );

depends_on:
mysql:
condition: service_healthy
healthcheck:
test: ["CMD", "bash" ,"-c", "[ -f /var/www/html/wp-config.php ]"]
interval: 4s
timeout: 1s
retries: 30

wpcli:
image: 'public.ecr.aws/docker/library/wordpress:cli'
user: 33:33 # Fixes permissions issues with writing files
volumes:
- ./.buildkite/setup-test-site.sh:/setup-test-site.sh:ro
- ./.wordpress/wp-config.php:/var/www/html/wp-config.php:ro
- ./test_credentials:/tmp/test_credentials
Copy link
Contributor

Choose a reason for hiding this comment

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

If I am understanding the setup correctly, it feels a bit backwards to me.

In test-server, we remove the file, re-create it, then we deploy it to Docker which will get it populated in .buildkite/setup-test-site.sh.

I think any test credential that's created and used by Docker should stay in Docker and not get exposed or altered by the local environment. Similarly, any local test credential shouldn't be altered when a Docker command is run.

With the Docker setup, we probably don't need a local test credential, so we could maybe just keep everything in Docker and that's good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd agree with you if I could find a reasonable way to use a specific string as an Application-Specific Password. Unfortunately, wp-cli doesn't seem to offer this – it'll make you a token, but then you have to get it out of the container somehow, and this was the most straightforward way I could see to do that 🤷

In test-server, we remove the file, re-create it
That's a file ownership thing – I took a shortcut to make it so that the file would be present on-disk without the wpcli container being able to write the entire filesystem (which results in conflicts, because it needs its own WordPress installation that's not the same version we're trying to specify)

With the Docker setup, we probably don't need a local test credential

I'm thinking that if you want to run this locally you'd use this approach too – make test-server then run your e2e test suite. The test suite can just pick up the test_credentials file and use its authentication details to run the tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaah, we do need to take it out of the container, because we need to use it while running our tests. I kind of blanked on that, sorry.

I think we can probably change the name a bit to maybe hint at how this is handled by docker. Something like generated_test_credentials or even docker_generated_test_credentials?

environment:
WORDPRESS_DB_HOST: mysql
WORDPRESS_DB_USER: wordpress
WORDPRESS_DB_PASSWORD: wordpress
WORDPRESS_DB_NAME: wordpress
depends_on:
mysql:
condition: service_healthy
wordpress:
condition: service_healthy
entrypoint: ["/setup-test-site.sh"]
profiles:
- donotstart

mysql:
image: 'public.ecr.aws/docker/library/mysql:8.0'
ports:
- '3306:3306'
environment:
MYSQL_DATABASE: 'wordpress'
MYSQL_USER: 'wordpress'
MYSQL_PASSWORD: 'wordpress'
MYSQL_RANDOM_ROOT_PASSWORD: true
healthcheck:
test: ["CMD", "mysqladmin" ,"ping", "-h", "localhost"]
interval: 4s
timeout: 1s
retries: 30
5 changes: 2 additions & 3 deletions native/swift/Sources/wordpress-api/Posts/API+ListPosts.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,14 @@ extension WordPressAPI {

/// Fetch a list of posts
///
/// If you're only interested in fetching a specific page, this is a good method for that – if you want to sync all records, consider using the
/// overload of this method that returns `PostObjectSequence`.
/// If you're only interested in fetching a specific page, this is a good method for that – if you
/// want to sync all records, consider using the overload of this method that returns `PostObjectSequence`.
public func listPosts(params: PostListParams = PostListParams()) async throws -> PostListResponse {
let request = self.helper.postListRequest(params: params)
let response = try await perform(request: request)
return try parsePostListResponse(response: response)
}


/// A good way to fetch every post (you can still specify a specific offset using `params`)
///
public func listPosts(params: PostListParams = PostListParams()) -> PostObjectSequence {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ import Foundation
import wordpress_api_wrapper

extension PostObject: Identifiable {
// swiftlint:disable identifier_name
var ID: any Hashable {
self.id
}
// swiftlint:enable identifier_name
}

public typealias PostCollection = [PostObject]
Expand All @@ -17,6 +19,10 @@ public struct PostObjectSequence: AsyncSequence, AsyncIteratorProtocol {
private var posts: [PostObject] = []
private var nextPage: WpNetworkRequest?

enum Errors: Error {
case unableToFetchPosts
}

init(api: WordPressAPI, initialParams: PostListParams) {
self.api = api
self.nextPage = api.helper.postListRequest(params: initialParams)
Expand All @@ -41,7 +47,7 @@ public struct PostObjectSequence: AsyncSequence, AsyncIteratorProtocol {
if let postList = parsedResponse.postList {
self.posts.append(contentsOf: postList)
} else {
abort() // TODO: Not sure if this should be an error
throw Errors.unableToFetchPosts
}

if let nextPageUri = parsedResponse.nextPage {
Expand Down
Loading