Page MenuHomePhabricator

[2hrs] PageImages code should have documentation warning developers about caching implications
Closed, ResolvedPublic

Description

There is no inline documentation or note in the README in the PageImages extension about how PageImages are populated and what developers should be wary of when working with this codebase. Let's add some to protect against incidents such as https://wikitech.wikimedia.org/wiki/Incident_documentation/20161202-20161201-PageImages

Details

Related Gerrit Patches:
mediawiki/extensions/PageImages : masterDocument page property names

Event Timeline

Jdlrobson created this task.Dec 2 2016, 8:48 PM
Restricted Application added a subscriber: Aklapper. Β· View Herald TranscriptDec 2 2016, 8:48 PM
Jdlrobson updated the task description. (Show Details)Dec 2 2016, 8:49 PM
bmansurov updated the task description. (Show Details)Dec 7 2016, 5:27 PM
β€’ jhobs triaged this task as Medium priority.Dec 7 2016, 5:28 PM
β€’ jhobs set the point value for this task to 0.5.
β€’ jhobs moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
ovasileva added a subscriber: ovasileva.

@bmansurov - did we identify any next steps on this?

@ovasileva could you clarify what you mean? I think this task is self-containing.

Jdlrobson renamed this task from PageImages code should have documentation warning developers about caching implications to [2hrs] PageImages code should have documentation warning developers about caching implications.Jan 18 2017, 6:33 PM
Jdlrobson removed the point value for this task.
Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-90-🍌 board.

Change 333805 had a related patch set uploaded (by Jdlrobson):
Document page property names

https://gerrit.wikimedia.org/r/333805

I've +2'ed the above patch. Any plans to update the README file as well, @Jdlrobson?

Change 333805 merged by jenkins-bot:
Document page property names

https://gerrit.wikimedia.org/r/333805

It seemed an inline comment was most appropriate here.

We should document the config variables in a readme and how they behave but I don't see how this would help protect us against license issues like that in the incident since there is no config switch that impacts it.

phuedx added a subscriber: phuedx.Jan 25 2017, 2:53 PM

We should document the config variables in a readme and how they behave but I don't see how this would help protect us against license issues like that in the incident since there is no config switch that impacts it.

This task is about documenting caching implications gotchas around the licensing-related config variables and not necessarily licensing issues themselves. A note in the README seems appropriate.

Jdlrobson added a comment.EditedJan 25 2017, 3:06 PM

There are no licensing related config; that's my point. The inline comment is most appropriate in my opinion then a out of context note in a read me... Which is done.

Mibad. I'd assumed that the free-by-default behaviour was flagged…

^ Thinking about my assumptions: am I correct in assuming that we announced the new default on a mailing list?

I couldn't find an announcement, though this is likely not finding the right Google incantation. The most recent changes have been discussed a number of times on reading-wmf and well documented on mediawiki.org.

I think this task can be considered done since the new behaviour is the default and it's not flagged.

phuedx reassigned this task from Jdlrobson to bmansurov.Jan 26 2017, 2:04 PM

I suggest we discuss the new default as part of sign off for T152216.

bmansurov closed this task as Resolved.Jan 27 2017, 7:02 PM
bmansurov removed bmansurov as the assignee of this task.