Add CI #35

jkmassel · 2024-03-06T07:05:45Z

Adds some initial CI tooling for Rust + Swift – I'm not sure Kotlin makes sense until we start down the path of building a platform-independent implementation?

This also includes boilerplate for Docker-based WordPress test sites (currently using the WP Theme Unit Test content, but we can add more if needed), but right now they're not doing anything useful.

Style fixes

oguzkocer

Thanks for taking care of this @jkmassel, overall it looks great!

I've left a bunch of comments, but they are mostly questions/nitpicks. Curious to hear your thoughts on them 🙇‍♂️

oguzkocer · 2024-03-06T17:01:38Z

.buildkite/pipeline.yml

+  plugins: &common_plugins
+    - automattic/a8c-ci-toolkit#2.17.0
+  # Common environment values to use with the `env` key.
+  env: &common_env


Since this is a multi-platform project and the IMAGE_ID is Swift specific value, it might be worth being more specific with the name.

Unfortunately we can't customize the environment variable name – rather than do too much weirdness, I've just pulled down the image ID into the step that needs it for now (see ebe78fc)

I thought the common_env bit was a given name which then gets referred to in the rest of the yml. Could you clarify why we can't customize the name? Do we have tooling that expects it?

Ohh, yeah that part could be renamed for sure.

If we need to re-introduce it, it could be mac_env or something

.buildkite/pipeline.yml

oguzkocer · 2024-03-06T17:08:36Z

.buildkite/pipeline.yml

+  # Common environment values to use with the `env` key.
+  env: &common_env
+    IMAGE_ID: xcode-15.2
+


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

oguzkocer · 2024-03-06T17:18:37Z

.buildkite/setup-test-site.sh

@@ -0,0 +1,38 @@
+#!/bin/bash


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

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!

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 🤔 )

oguzkocer · 2024-03-06T17:21:23Z

.buildkite/setup-test-site.sh

+	--skip-email
+
+## Ensure URLs work as expected
+wp rewrite structure '/%year%/%monthnum%/%postname%/'


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?

This is the closest thing to a "default" that the ecosystem has IMHO.

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

oguzkocer · 2024-03-06T17:32:42Z

Makefile

+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)


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?

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:

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.

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.

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. 😞

oguzkocer · 2024-03-06T17:34:06Z

.gitignore

 test_credentials
+/logs
+
+# CI Cache


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.

oguzkocer · 2024-03-06T17:41:25Z

docker-compose.yml

+        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


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?

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?

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?

oguzkocer · 2024-03-06T17:42:18Z

docker-compose.yml

+            WORDPRESS_DB_PASSWORD: wordpress
+            WORDPRESS_DB_NAME: wordpress
+            WORDPRESS_CONFIG_EXTRA: |
+                # Allow application passwords to be used without HTTPS


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

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

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.

oguzkocer

Approving with some pending non-blocking comments, so @jkmassel can merge it when it's ready.

crazytonyli · 2024-03-07T20:59:27Z

.buildkite/hooks/post-command

+docker-compose logs wordpress > logs/wordpress.log
+docker-compose logs mysql > logs/mysql.log
+
+buildkite-agent artifact upload "logs/*.log"


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:

Addressed in cba14ff, thanks!!

jkmassel · 2024-03-11T16:52:26Z

There's other stuff we'll want to address, but I'm merging this for now to get it out of the way and allow for more focused adjustments as needed

jkmassel added 6 commits March 4, 2024 16:23

Add basic Rust lint + test

a586a38

Fix clippy issues

fa41f05

Build / Test / Lint Swift code

8e39b3a

Style fixes

Fix SwiftLint issues

867bb12

Disable test suite for now

e3e9af5

Add e2e tests

850427e

jkmassel added the Tooling label Mar 6, 2024

jkmassel requested a review from oguzkocer March 6, 2024 07:05

jkmassel self-assigned this Mar 6, 2024

A better way to allow application passwords

a9b234b

jkmassel mentioned this pull request Mar 6, 2024

Secret credentials support #33

Closed

jkmassel added 2 commits March 6, 2024 00:23

Remove end-to-end Swift tests

164edc6

Speed up server setup

aa531c2

jkmassel force-pushed the add/ci branch 5 times, most recently from 906e285 to a6776f2 Compare March 6, 2024 08:09

Have the test suite use test_credentials

7ca6619

jkmassel force-pushed the add/ci branch from a6776f2 to 7ca6619 Compare March 6, 2024 08:14

oguzkocer reviewed Mar 6, 2024

View reviewed changes

jkmassel added 2 commits March 6, 2024 12:37

Make it more obvious what IMAGE_ID means

ebe78fc

Code cleanup

06d8cf4

oguzkocer approved these changes Mar 6, 2024

View reviewed changes

oguzkocer added this to the 0.1 milestone Mar 7, 2024

oguzkocer mentioned this pull request Mar 7, 2024

Switch to UniFFI Procedural Macros #36

Merged

crazytonyli reviewed Mar 7, 2024

View reviewed changes

crazytonyli mentioned this pull request Mar 10, 2024

Add URLSession helpers to send request and parse response #38

Closed

jkmassel added 2 commits March 11, 2024 10:30

Don’t run the Docker hook outside AWS

cba14ff

Better error handling

d659624

More reliability improvements

4e94f60

jkmassel merged commit eb23029 into trunk Mar 11, 2024

jkmassel deleted the add/ci branch March 11, 2024 16:52

This was referenced Apr 4, 2024

Set up Docker to run tests on specified WordPress version #24

Closed

Buildkite integration for Swift project #14

Closed

Buildkite integration for Rust project #12

Closed

Add CI #35

Add CI #35

Uh oh!

Conversation

jkmassel commented Mar 6, 2024

Uh oh!

oguzkocer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oguzkocer Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oguzkocer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkmassel commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oguzkocer Mar 6, 2024 •

edited

Loading