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

RocketCDN CNAME change #7027

Open
piotrbak opened this issue Oct 10, 2024 · 12 comments · May be fixed by #7095
Open

RocketCDN CNAME change #7027

piotrbak opened this issue Oct 10, 2024 · 12 comments · May be fixed by #7095
Assignees
Labels
module: CDN module: rocketcdn needs: documentation Issues which need to create or update a documentation type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@piotrbak
Copy link
Contributor

Is your feature request related to a problem? Please describe.
In the upcoming version we'll need to change the RocketCDN CNAMEs automatically

Describe the solution you'd like
The Acceptance Criteria is described here:
https://www.notion.so/wpmedia/RocketCDN-CNAME-change-d72cd2baa88b49b7b4cb65b348440564?pvs=4

@piotrbak piotrbak added module: CDN module: rocketcdn needs: grooming type: enhancement Improvements that slightly enhance existing functionality and are fast to implement labels Oct 10, 2024
@piotrbak piotrbak added this to the 3.17.2 milestone Oct 10, 2024
@piotrbak piotrbak added the needs: documentation Issues which need to create or update a documentation label Oct 10, 2024
@Khadreal
Copy link
Contributor

Khadreal commented Oct 17, 2024

Scope a solution ✅

Add new method to handle the notice for RocketCDN here and the newly created method to the admin_notices filter here.

The method created above should only be displayed based on this conditions here

Then in this file add a new method

public function update_cdn_name( $new_version, $old_version ): void {
		if ( version_compare( $old_version, '3.17.2', '>' ) ) {
			return;
		}
                delete_transient('rocketcdn_status');

		$this->api_client->get_subscription_data();
	}

Add the APiClient as constructor to Subscriber

Finally, add the method created as a callback to wp_rocket_upgrade

Add test, update existing ones.

Estimation

[S]

@MathieuLamiot
Copy link
Contributor

As the approach is based on the RocketCDN API, the plugin release with this change must happen after this is released: https://github.com/wp-media/rocket-cdn/issues/546
Depending on the delay on the service team side, we might want to take this out of 3.17.2

@MathieuLamiot MathieuLamiot removed this from the 3.17.2 milestone Oct 18, 2024
@MathieuLamiot
Copy link
Contributor

Removing the 3.17.2 milestone as the RocketCDN API won't be ready for 3.17.2.

@wordpressfan
Copy link
Contributor

We'll display a (success/warning/info) dismissible notice in the WP Rocket plugin settings page

This point is still not handled by this grooming, correct? @Khadreal

@Khadreal
Copy link
Contributor

It's the notice in WPR is dismissible by default, so I don't think it's needed to cover that again.

@wordpressfan
Copy link
Contributor

I'm not talking about the dismiss, I'm talking about the notice itself, I believe we need to show it in WPR settings along with rocketCDN settings page.
cc @piotrbak

@piotrbak
Copy link
Contributor Author

@wordpressfan Yes, the notice should be there.

@DahmaniAdame I think we should display a blue, notification notice. WDYT?

@DahmaniAdame
Copy link
Contributor

@piotrbak I agree. It's informational and non-critical. Blue will do.

We can use orange if we need a second message later in the process if we notice that the former CNAME is still heavily used.

@MathieuLamiot
Copy link
Contributor

Grooming to complete with the notice, as pointed out above, before moving it to Grooming to Review.

@MathieuLamiot MathieuLamiot added this to the 3.17.3 milestone Oct 23, 2024
@remyperona
Copy link
Contributor

Everything related to RocketCDN should be handled in RocketCDN classes. The update of the CDN cname can be done in the DataManagerSubscriber class, which already has the API client as a dependency.

We also need to take into account saving the old CDN URL to be able to display it in the notice.

Moving it back to grooming to do.

@wordpressfan wordpressfan added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels Nov 4, 2024
@wordpressfan
Copy link
Contributor

Grooming

Will take the grooming mentioned above by @Khadreal with some small changes.

Proposed Solution

In this subscriber:

self::CRON_EVENT => 'maybe_disable_cdn',

Add a new callback to that array update_cdn_name, this method will be triggered with update action wp_rocket_upgrade, and will save the old cdn url then force reloading subscription data again as below:

public function update_cdn_name( $new_version, $old_version ): void {
	if ( version_compare( $old_version, '3.17.2', '>' ) ) {
		return;
	}
	
	$old_subscription_data = $this->api_client->get_subscription_data();
	
	if ( ! $old_subscription_data['is_active'] || empty( $old_subscription_data['cdn_url'] ) ) {
		return;
	}
	
	update_option( 'rocketcdn_old_url', $old_subscription_data['cdn_url'] );

	delete_transient('rocketcdn_status');

	$this->api_client->get_subscription_data();
}

For showing the notice, We will create a new view file here:

https://github.com/wp-media/wp-rocket/blob/d70151f977efdf2e57a5c6c863ffd8f31a70a905/inc/Engine/CDN/RocketCDN/views

with the notice HTML contents, then we will create a new method here:

class NoticesSubscriber extends Abstract_Render implements Subscriber_Interface {

To get the old cdn url from the option rocketcdn_old_url mentioned above and the new cdn url, then will call the generate method to show the view file that we created above.

We may need to remove the option rocketcdn_old_url with notice dismiss, also with plugin removal.

Then adjust tests.

Effort

[S]

@wordpressfan wordpressfan removed the GROOMING IN PROGRESS Use this label when the issue is currently being groomed. label Nov 4, 2024
@Khadreal
Copy link
Contributor

Khadreal commented Nov 4, 2024

LGTM

@wordpressfan wordpressfan self-assigned this Nov 6, 2024
@wordpressfan wordpressfan linked a pull request Nov 7, 2024 that will close this issue
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: CDN module: rocketcdn needs: documentation Issues which need to create or update a documentation type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants