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

feat: Automatic sqlite-database-integration upgrade #136

Merged
merged 8 commits into from
May 21, 2024

Conversation

dcalhoun
Copy link
Member

@dcalhoun dcalhoun commented May 15, 2024

Resolves https://github.com/Automattic/dotcom-forge/issues/6939.
Fixes https://github.com/Automattic/dotcom-forge/issues/6922.
Relates to https://github.com/Automattic/dotcom-forge/issues/6598.
Relates to #132.
Relates to https://github.com/Automattic/local-environment/pull/245 and https://github.com/Automattic/local-environment/pull/228.

Proposed Changes

  • Update the sqlite-databse-integration installation to the latest release tag on app start.
  • Update the sqlite-databse-integration installation to the latest release tag on site server start.

Testing Instructions

Important

In order to observe the sqlite-integration-plugin upgrade, you must first install a legacy version of the plugin.

  1. Check out the trunk branch.

  2. Download a legacy version of sqlite-integration-plugin — e.g., v2.1.9.

  3. "Install" the legacy version by replacing the existing copy found in the following locations.

    Note: naming the directory sqlite-database-integration-main (legacy name) would simulate testing the rollout of these changes to existing installs/sites; naming the directory sqlite-database-integration (new name) would simulate testing an additional SQLite upgrade (second, third, etc) after a previous install (first SQLite upgrade, new site creation using these changes, etc).

    • Bundled Installation: This is found in the cloned repository at wp-files/sqlite-database-integration-main.
    • Downloaded Installation: On macOS, this is found in ~/Library/Application\ Support/Studio/server-files/sqlite-database-integration-main.
  4. Start the app server. (Note, you should not run npm install at this point as it would update the legacy bundled installation.)

  5. Create a new site ("Legacy Site").

  6. Verify the Legacy Site's wp-content/mu-plugins directory contains the legacy version of the plugin.

1: Site actions remain stable without a network connection

  1. Check out the proposed changes.
  2. Run npm install to download/bundle the latest version of sqlite-database-integration.
  3. Disable your internet connection.
  4. Start the app server.
  5. Create a new site.
  6. Verify no errors are thrown during site creation or site server start.

2: New sites leverage the latest version

  1. Check out the proposed changes.
  2. Start the app server.
  3. Create a new site.
  4. Verify the sqlite-integration-plugin version found in the site's wp-content/mu-plugins directory is the latest release.

3: Existing sites upgrade when started

  1. Check out the proposed changes.
  2. Start the app server.
  3. Start the Legacy Site's server.
  4. Verify the sqlite-integration-plugin version found in the site's wp-content/mu-plugins directory is the latest release.

4: Woocommerce loads without database query errors

  1. Check out the proposed changes.
  2. Start the app server.
  3. Install and activate the WooCommerce plugin (Steps outlined in https://github.com/Automattic/dotcom-forge/issues/6922).
  4. Verify no database query errors are logged and the plugin page loads successfully. Note: If MailPoet is installed and activated, errors may display.

5: UpdraftPlus loads without database query errors

Test case removed as the proposed changes do not address all errors.

6: Fresh Studio installs successfully create and start sites

  1. Delete all existing Studio sites.
  2. Clear out the bundled and downloaded sqlite-database-integration installations.
  3. Run npm install to download/bundle the latest version of sqlite-database-integration.
  4. Start the app server.
  5. Create a new site.
  6. Verify the sqlite-integration-plugin version found in the site's wp-content/mu-plugins directory is the latest release.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@dcalhoun dcalhoun added the [Type] Enhancement Improvement upon an existing feature label May 15, 2024
@dcalhoun dcalhoun self-assigned this May 15, 2024
Improve stability and enable version comparisons for future upgrades.
Ensure new sites receive the latest sqlite-database-integration fixes
and improvements.
@dcalhoun dcalhoun force-pushed the feat/automatic-sqlite-upgrade branch from fe6035d to 2db6c54 Compare May 16, 2024 23:59
Ensure existing sites receive the latest fixes and features.
If version comparison fails, assume the installed version is the latest.
For example, fetching the latest version while offline will result in a
comparison failure.
Avoid repeatedly fetching the latest versions each time a site starts.
@dcalhoun dcalhoun force-pushed the feat/automatic-sqlite-upgrade branch from 2db6c54 to f0aa6f2 Compare May 17, 2024 00:04
@dcalhoun dcalhoun changed the title feat: Automatic SQLite upgrade feat: Automatic sqlite-database-integration upgrade May 17, 2024
scripts/download-wp-server-files.ts Show resolved Hide resolved
src/ipc-handlers.ts Show resolved Hide resolved
src/lib/sqlite-versions.ts Outdated Show resolved Hide resolved
vendor/wp-now/src/download.ts Outdated Show resolved Hide resolved
@dcalhoun dcalhoun marked this pull request as ready for review May 17, 2024 00:41
@dcalhoun dcalhoun requested review from a team May 17, 2024 00:41
@dcalhoun
Copy link
Member Author

Expanded the testing instructions in the description as I realized this will also fix https://github.com/Automattic/dotcom-forge/issues/6922 and https://github.com/Automattic/dotcom-forge/issues/6598.

Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

I tested all the scenarios and worked great in all of them.
I added a few comments. My only suggestion is to use the same source, wp.org or GitHub, for the bundled version and the start/updated version.

Updated existing SQLite version from 2.1.7 to 2.1.10

sqlite-existing-site.mp4
sqlite-new-site.mp4
sqlite-woo.mp4
sqlite-updraft-plus.mp4

scripts/download-wp-server-files.ts Show resolved Hide resolved
src/ipc-handlers.ts Show resolved Hide resolved
src/lib/sqlite-versions.ts Show resolved Hide resolved
src/lib/sqlite-versions.ts Outdated Show resolved Hide resolved
We only ever install the latest version, so we do not need the
unnecessary complexity of fetching past versions. Also, this allows us
to use the same download source the `sqlite-database-integration` plugin
throughout the source.
@dcalhoun dcalhoun requested a review from sejas May 18, 2024 12:56
@fluiddot
Copy link
Contributor

Testing Instructions
UpdraftPlus loads without database query errors

I tested the plugin and I'm encountering the same JS exceptions outlined in https://github.com/Automattic/dotcom-forge/issues/6598:

ReferenceError: updraftlion is not defined

I presume that using the latest SQLite version solved database errors, but as far as I've tested, I think this PR won't solve the issue. The plugin doesn't seem to function properly, clicking the Backup Now button results in no actions.

@dcalhoun
Copy link
Member Author

I tested the [UpdraftPlus] plugin and I'm encountering the same JS exceptions outlined in Automattic/dotcom-forge#6598:

ReferenceError: updraftlion is not defined

Thank you for sharing this. From earlier testing, my perception was that this error was present for non-Playground sites as well, but it appears I was mistaken. I'll debug this further.

The `-main` suffix is no longer accurate now that we rely upon release
tags rather than downloading the latest development branch.
Copy link
Member Author

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

Thank you @sejas and @fluiddot for reviewing and helping me test this thoroughly. Apologies for the overlooked cases. I believe this is ready for review again.

  1. I updated the test case 4 (WooCommerce) to note that errors remain if MailPoet is activated.
  2. I removed test case 5 (UpdraftPlus) as the proposed changes do not address all the errors.
  3. I added test case 6 to ensure that fresh installations of Studio — which lack sqlite-database-integration plugin installations entirely — work as expected.

src/setup-wp-server-files.ts Show resolved Hide resolved
@dcalhoun dcalhoun requested a review from fluiddot May 20, 2024 17:06
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

LGTM 🎊 ! I've tested the different test scenarios and worked as expected. Thanks @dcalhoun for adding this feature 🏅 !

I added a comment that shouldn't block the PR but might be interesting to consider.

Nitpick: In the Important section of Testing Instructions, there are references to the old SQLite filename sqlite-database-integration-main. I presume it should be sqlite-database-integration, without the suffix -main`.

function getSqliteVersionFromInstallation( installationPath: string ): string {
let versionFileContent = '';
try {
versionFileContent = fs.readFileSync( path.join( installationPath, 'load.php' ), 'utf8' );
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we try to avoid using the sync version of FS functions to prevent blocking the main thread. I wonder if we could use the asynchronous one.

@dcalhoun
Copy link
Member Author

Nitpick: In the Important section of Testing Instructions, there are references to the old SQLite filename sqlite-database-integration-main. I presume it should be sqlite-database-integration, without the suffix -main.

@fluiddot Thanks for the note!

My thought is that testing old-version SQLite installations with and without the -main suffix is relevant. The former applies for existing users receiving upgrades to existing installations with -main. The latter applies for users receiving upgrades to existing installations without -main (e.g., upgrading SQLite a second time or installing Studio for the first time after this change ships).

I updated the documentation account for both of these nuanced circumstances.

@dcalhoun dcalhoun merged commit d296d88 into trunk May 21, 2024
14 checks passed
@dcalhoun dcalhoun deleted the feat/automatic-sqlite-upgrade branch May 21, 2024 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improvement upon an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants