Page MenuHomePhabricator

[REQUEST] Review updated test ESI comment injection patch
Closed, ResolvedPublic1 Estimated Story Points

Description

What teams or group is this for?
FR-Tech and (indirectly) Traffic.

Who is your main point of contact and contact preference?
Contacts: @AndyRussG, @XenoRyet, @greg.
Communication preference: via Phabricator works for us.

What are the details of your request? Include relevant timelines or deadlines
Request: review of new patch for temporary injection of ESI test string.
We'd like someone familiar current code for skins and base HTML generation to please have a quick look at this patch, just to be sure it's not harmful in some way we're not aware of.
Timeline: relatively soon if possible. Apologies for the delay in making this request.
We'll submit another request for help designing a more permanent solution for adding a banner ESI string to the base HTML at the correct location.

How does the request fit within the departmental or foundation priorities?
This supports tests of ESI in Varnish. ESI may be eventually used to eliminate the banner bump. (I don't know if it could be used later on for other purposes, too.) For details, please see T320734, T308799, T303075 and T298733.

Is this request urgent or time sensitive?
Ish? It'd be great to get this out soon (though we also recognize we've taken a while to find the time to dig in and send this request).

Event Timeline

ovasileva lowered the priority of this task from High to Medium.Nov 21 2022, 5:41 PM
Jdlrobson set the point value for this task to 1.Nov 21 2022, 6:43 PM

Discussed with @greg and will aim to complete the review in January

Hi @ovasileva, @greg :)

Quick update: the patch in question was looked over by other fr-tech engineers and we agreed it seems extremely safe. So, we merged it to master, which meant it has been deployed to the beta cluster, but not to production, since December 15th.

(Because of CentralNotice's unusual deploy setup, the master branch only goes out to production after being manually merged to the wmf_deploy branch.)

The patch has been running without any issues (that I can see) on the beta cluster since that date. We'd like to merge it along with other accumulated changes on CentralNotice's master branch to the wmf_deploy branch, which would put things on the deploy train, possibly even for this week? We'd watch production for any possible issues following train deploys over the week.

I hope this is OK! Please let us know if you have any concerns about this plan!! Thanks so so much in advance.

The BeforePageDisplay hook approach looks a lot better than the SiteNotice one @AndyRussG (enabling $wgMinervaEnableSiteNotice would have likely caused a lot of issues with user notices on mobile). If you need the ESI string to go inside banners in future, I'd suggest we do some refactoring of Minerva to achieve that. Glad to hear you are unblocked!

The BeforePageDisplay hook approach looks a lot better than the SiteNotice one @AndyRussG (enabling $wgMinervaEnableSiteNotice would have likely caused a lot of issues with user notices on mobile). If you need the ESI string to go inside banners in future, I'd suggest we do some refactoring of Minerva to achieve that. Glad to hear you are unblocked!

Got it, thanks!! Yes, if the ESI banner service goes ahead (still at least contingent upon Varnish's ESI implementation working at scale) we will indeed need the ESI string to go where banners currently are injected in the base HTML. I guess details would be discussed elsewhere in that case, but I can quickly preview the idea I have so far: add a a parameter to the SiteNoticeBefore hook for handlers to provide "extra" HTML content to output at that location (regardless of whether or not the Site Notice itself is shown). Just an initial idea, no idea if it's a good one. :)

Thanks again! :)

Can this ticket be closed @AndyRussG given I think you took another direction?

Can this ticket be closed @AndyRussG given I think you took another direction?

Yep! Thanks so much! :)

Jdlrobson claimed this task.