[CICD-761] Generate a dynamic excludes file list for rsync #40

mike-day · 2024-12-04T16:32:00Z

JIRA Ticket

Bug description

Customer reports that mu-plugins are being deleted when deploying with the GitHub Action. The plugins that are mentioned as being deleted are already part of our global excludes list in wpengine/site-deploy.

Reported in: wpengine/github-action-wpe-site-deploy#91

Background

We worked through several options for this ticket before landing on this implementation.

In #37, we tried forcing files to adhere to a specific structure expected by the remote target (making a relative remote), but felt the approach ended up being a bit risky with re-writing directory structure on each sync.

In #39, we tried removing relative path prepending in the exclude.txt file, but the approach was deemed a risk for affecting users that deploy to the root directory of a site (the paths in exclude.txt were treated like globs, meaning custom files could be unintentionally excluded).

The approach outlined below came up while solutioning in a review session, and we believe it is another good long-term option that reduces such risks.

What Are We Doing Here

This approach creates a excludes list for rsync on the fly, with the path for files in the excludes list inferred from the REMOTE_PATH environment variable.

No exclude.txt is created — instead, the list is output to stdout before being used in a process substitution. This provides a temp file rsync can consume before it is destroyed at the end of the process.

Outputs the list to stdout, which is called with process substitution in the initial rsync command. Also disables debug mode for the block calling the generate function. This prevents unwanted debug output (specific to the excludes list generation) from being shown.

Moves the test-plugin (used by e2e test) to a wp-content dir for reuse as a fixture in unit testing.

Resolves a warning about unused variables in the common file.

changeset-bot · 2024-12-04T16:32:04Z

🦋 Changeset detected

Latest commit: cec397d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@wpengine/site-deploy	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

mike-day · 2024-12-04T16:34:37Z

.github/workflows/e2e-deploy.yml

@@ -19,15 +19,15 @@ jobs:
    steps:
      - uses: actions/checkout@v3
      - name: Bump test plugin version number
-        run: sed -i 's/0.0.1/0.0.2/' tests/data/plugins/test-plugin/test-plugin.php
+        run: sed -i 's/0.0.1/0.0.2/' tests/data/wp-content/plugins/test-plugin/test-plugin.php


The plugin used for e2e tests was moved to reside under a wp-content parent dir. The reasoning was to make the file structure more closely resemble a WordPress install, making the plugin reusable for unit testing.

mike-day · 2024-12-04T16:39:11Z

entrypoint.sh

+  # Generate the excludes list before using the output with rsync
+  local exclude_from; exclude_from="$(generate_exclude_from)"


I found it preferable to generate the output in a variable here.

The text works more reliably when quoted, and while calling the function directly in the process substitution works (eg, <(echo "generate_exclude_from")), it produces warnings about a needless echo.

mike-day · 2024-12-04T16:44:14Z

entrypoint.sh

  #create multiplex connection 
  ssh -nNf -v -i "${WPE_SSHG_KEY_PRIVATE_PATH}" -o StrictHostKeyChecking=no -o ControlMaster=yes -o ControlPath="$SSH_PATH/ctl/%C" "$WPE_FULL_HOST"
  echo "!!! MULTIPLEX SSH CONNECTION ESTABLISHED !!!"

  set -x
-  rsync --rsh="ssh -v -p 22 -i ${WPE_SSHG_KEY_PRIVATE_PATH} -o StrictHostKeyChecking=no -o 'ControlPath=$SSH_PATH/ctl/%C'" "${FLAGS_ARRAY[@]}" --exclude-from='/exclude.txt' --chmod=D775,F664 "${SRC_PATH}" "${WPE_DESTINATION}"
+  rsync --rsh="ssh -v -p 22 -i ${WPE_SSHG_KEY_PRIVATE_PATH} -o StrictHostKeyChecking=no -o 'ControlPath=$SSH_PATH/ctl/%C'" "${FLAGS_ARRAY[@]}" --exclude-from=<( { { set +x; } 2>/dev/null; echo "$exclude_from"; } ) --chmod=D775,F664 "${SRC_PATH}" "${WPE_DESTINATION}"


Using a block inside the process substitution and disabling debug mode prevents unwanted output. Sending errors to /dev/null also prevents ++ set +x from being added to the output.

The result is debugging output that is identical to how it was previously (with the exception of --exclude-from=/dev/fd/63, which is referring to the temp file path created by the process substitution).

mike-day · 2024-12-04T16:57:20Z

exclude-from.sh

+  case "$REMOTE_PATH" in
+    "wp-content")
+      base_path=""
+      mu_dir_path="mu-plugins/"
+      ;;
+    "wp-content/mu-plugins")
+      base_path=""
+      mu_dir_path=""
+      ;;
+  esac


Check me on this, but I believe these represent the only cases in which we would need to modify the paths.

If the user is deploying to other expected directories (eg, wp-content/plugins or wp-content/themes), the full relative paths are included.

While the above has no negative effect, we could consider further modifying the excludes list at that point to completely remove the WP and WPE-specific file paths. I did consider implementing that, but it felt a bit like overkill 🙃

I wonder if wp-content/ and wp-content/mu-plugins (/ appended) should also be matched 🤔

It seems like the deploy would likely work with either as the REMOTE_PATH, so those might need to be added for comparison here.

To-do: test with and without appended slash for REMOTE_PATH.

To-do: test with rsync destructive mode to confirm excludes work from the other direction (remote to local). Might mostly refer to a need for a new test case.

mike-day · 2024-12-04T17:00:14Z

exclude-from.sh

+  {
+    IFS= read -r base_path
+    IFS= read -r mu_dir_path
+  } <<< "$(determine_exclude_paths)"


This is a shorthand approach for assigning local variables that respect "" as a valid value.

In other words, calling read -r base_path mu_dir_path "$(determine_exclude_paths)" can overwrite the first value with the second if only the first value will be evaluated as "".

mike-day · 2024-12-04T17:03:29Z

tests/test_exclude_from.sh

+test_rsync_with_excludes() {
+  export REMOTE_PATH=$1
+  echo -e "${BLUE}--- REMOTE_PATH: '$REMOTE_PATH'${NC}"
+
+  local exclude_from; exclude_from="$(generate_exclude_from)"
+  local fixture_path="$SCRIPT_DIR/data"
+
+  # Capture the output of the rsync command
+  output=$(rsync --dry-run --verbose --recursive --exclude-from=<(echo "$exclude_from") "$fixture_path" .)
+
+  # Check if the output contains the expected file
+  if echo "$output" | grep -q "test-plugin.php"; then
+    echo -e "${GREEN}Test passed for rsync syncing files: test-plugin.php included.${NC}"
+  else
+    echo -e "${RED}Test failed for rsync syncing files: test-plugin.php excluded.${NC}"
+  fi
+
+  # Check that the output does not contain the excluded file
+  if echo "$output" | grep -q "mu-plugin.php"; then
+    echo -e "${RED}Test failed for rsync parsing excludes: mu-plugin.php included.${NC}"
+  else
+    echo -e "${GREEN}Test passed for rsync parsing excludes: mu-plugin.php excluded.${NC}"
+  fi
+}


While this is very simplistic, it confirms that basic includes/excludes are working as expected with this new implementation and rsync.

Further testing of entrypoint can be much more thorough when we have stubbing figured out.

Also moves common.sh to new utils dir.

Include a dockerfile for test image

mike-day · 2025-01-08T16:52:20Z

Closed in favor of #42

mike-day added 3 commits December 3, 2024 22:21

[CICD-761] Add tests for verifying rsync file inclusion and exclusion

9f7ae31

Moves the test-plugin (used by e2e test) to a wp-content dir for reuse as a fixture in unit testing.

[CICD-761] Explicitly export color globals

3351cbd

Resolves a warning about unused variables in the common file.

mike-day requested a review from a team as a code owner December 4, 2024 16:32

mike-day commented Dec 4, 2024

View reviewed changes

mike-day force-pushed the CICD-761-try-dynamic-exclude-from branch from 39787b5 to 3351cbd Compare December 4, 2024 18:12

mike-day added 12 commits December 5, 2024 11:10

[CICD-761] Bump version with patch update

5abebbf

[CICD-761] Update the test fixtures dir structure

2c6cf5c

[CICD-761] Create tests/utils dir

5105800

Also moves common.sh to new utils dir.

[CICD-761] Create tests/helpers dir

c475b09

Also moves common.sh to new utils dir.

[CICD-761] e2e deploy back to original data path

53c0e65

[CICD-761] Update verbiage in fixture

37e4056

[CICD-761] Create print_mount_paths util

22e4b7f

[CICD-761] Move functions to utils

a8ed10c

[CICD-761] WIP Create generate_path_excludes util

a71151a

Include a dockerfile for test image

[CICD-761] Generate the excludes dynamically in entrypoint

3b3904a

[CICD-761] Move change_working_dir helper

eb6bfab

[CICD-761] Move to a simpler approach for unit tests

cec397d

mike-day mentioned this pull request Dec 18, 2024

[CICD-761] Generate dynamic excludes for rsync (part deux) #42

Merged

mike-day closed this Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CICD-761] Generate a dynamic excludes file list for rsync #40

[CICD-761] Generate a dynamic excludes file list for rsync #40

mike-day commented Dec 4, 2024 •

edited

Loading

changeset-bot bot commented Dec 4, 2024 •

edited

Loading

mike-day Dec 4, 2024

mike-day Dec 4, 2024 •

edited

Loading

mike-day Dec 4, 2024 •

edited

Loading

mike-day Dec 4, 2024 •

edited

Loading

mike-day Dec 4, 2024

mike-day Dec 5, 2024

mike-day Dec 5, 2024 •

edited

Loading

mike-day Dec 4, 2024

mike-day Dec 4, 2024

mike-day commented Jan 8, 2025

		# Generate the excludes list before using the output with rsync
		local exclude_from; exclude_from="$(generate_exclude_from)"

[CICD-761] Generate a dynamic excludes file list for rsync #40

[CICD-761] Generate a dynamic excludes file list for rsync #40

Conversation

mike-day commented Dec 4, 2024 • edited Loading

JIRA Ticket

Bug description

Background

What Are We Doing Here

changeset-bot bot commented Dec 4, 2024 • edited Loading

🦋 Changeset detected

mike-day Dec 4, 2024

Choose a reason for hiding this comment

mike-day Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

mike-day Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

mike-day Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

mike-day Dec 4, 2024

Choose a reason for hiding this comment

mike-day Dec 5, 2024

Choose a reason for hiding this comment

mike-day Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

mike-day Dec 4, 2024

Choose a reason for hiding this comment

mike-day Dec 4, 2024

Choose a reason for hiding this comment

mike-day commented Jan 8, 2025

mike-day commented Dec 4, 2024 •

edited

Loading

changeset-bot bot commented Dec 4, 2024 •

edited

Loading

mike-day Dec 4, 2024 •

edited

Loading

mike-day Dec 4, 2024 •

edited

Loading

mike-day Dec 4, 2024 •

edited

Loading

mike-day Dec 5, 2024 •

edited

Loading