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

Sync Package: use Full_Sync_Immediately by default #14357

Merged
merged 2 commits into from
Jan 16, 2020

Conversation

roccotripaldi
Copy link
Member

@roccotripaldi roccotripaldi commented Jan 15, 2020

Fixes n/a

Changes proposed in this Pull Request:

  • The new full sync pattern introduced in Sync: Full Sync: Send immediately. #13963 should now be the default way to perform a full sync. We will still support the original full sync by allow sites to use a filter.
  • We want to get this into master early in the release cycle so we can test it as much as possible.
  • If sites need to use the now legacy full sync, they can do so with a filter:
function jetpack_full_sync_immediately_off( $modules )
	foreach ( $modules as $key => $module ) {
		if ( in_array( $module, [ 'Automattic\\Jetpack\\Sync\\Modules\\Full_Sync_Immediately', ] ) ) {
			$modules[ $key ] = 'Automattic\\Jetpack\\Sync\\Modules\\Full_Sync';
		}
	}
	return $modules;
}

add_filter( 'jetpack_sync_modules', 'jetpack_full_sync_immediately_off' );

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This optimizes the Sync package.

Testing instructions:

Heavy testing of this PR is not required to merge because we plan to test and optimize the new full sync pattern during the course of the release cycle. We do need to ensure that this PR causes no large fatals during a Full Sync.

  • Apply this patch to a Jetpack testing site
  • Disconnect the site if it's already connected
  • Re-connect the site, which should trigger a full sync
  • Ensure there are no fatal errors
  • Bonus: Set your Jetpack site's API base to wordpress.com sandbox and trigger a full sync. Watch as data flows on your sandbox, and ensure there are no fatals.

Proposed changelog entry for your changes:

  • Updates the Sync package with an optimized full sync.

The new full sync pattern introduced in #13963 should now be the default way to perform a full sync. We will still support the original full sync by allow sites to use a filter.
@roccotripaldi roccotripaldi added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Package] Sync labels Jan 15, 2020
@roccotripaldi roccotripaldi requested review from gravityrail and a team January 15, 2020 22:05
@jetpackbot
Copy link

jetpackbot commented Jan 15, 2020

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 0415d6d

Use the legacy full sync module for default tests.
@mdbitz
Copy link
Contributor

mdbitz commented Jan 16, 2020

This looks good Rocco, Just a note there was 2 extra spaces in your code snippet on reverting back to the Full Sync Module, so I modified your comment.

'Automattic\Jetpack\Sync\Modules\ Full_Sync_Immediately' => 'Automattic\Jetpack\Sync\Modules\Full_Sync_Immediately'

Copy link
Contributor

@mdbitz mdbitz left a comment

Choose a reason for hiding this comment

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

Verified Full_Sync_immediately is used for Full Syncs.

@jeherve jeherve added this to the 8.2 milestone Jan 16, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 16, 2020
@roccotripaldi roccotripaldi merged commit 0addb54 into master Jan 16, 2020
@roccotripaldi roccotripaldi deleted the full-sync-immediately-by-default branch January 16, 2020 14:02
@matticbot matticbot added [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 16, 2020
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Status] Needs Changelog labels Jan 17, 2020
jeherve added a commit that referenced this pull request Jan 17, 2020
jeherve added a commit that referenced this pull request Jan 28, 2020
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Sync [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants