make it possible to control whatsnew page showing or not via ship it, per-locale

RESOLVED FIXED

Status

Release Engineering
Release Automation
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bhearsum, Assigned: nthomas)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M-])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
The whatsnew page is controlled by "actions silent" in the patcher config. We could either add a text field on ship it that lets users add arbitrary actions (because "silent" isn't the only available one), or we could add a checkbox that deals with whatsnew explicitly.

The other question is what should be updating the patcher config? We could either add support to patcher-config-bump.pl for actions, or have release runner update the patcher config directly. I'm a bit torn on this part. I think having patcher-config-bump.pl do it makes the most sense, since it already modifies the config in a bunch of ways. If we do that, we need to add support for action modification to it, and figure out how we want to pass this information down the ship it -> buildbot-configs -> buildbot factory -> patcher-config-bump.pl chain.
(Assignee)

Comment 1

5 years ago
Is the scope here only a global on/off ? There was a query in email of being able to show whatsnew for only one/some locale(s).
(Reporter)

Comment 2

5 years ago
(In reply to Nick Thomas [:nthomas] from comment #1)
> Is the scope here only a global on/off ? There was a query in email of being
> able to show whatsnew for only one/some locale(s).

Huh, that part didn't click with me. As it's scoped now, this would be a global control. A per locale control would require patcher config format changes, I think? And then we'd have to figure out accept that input on Ship It in a reasonable manner.
Ideally, we would like to turn on /whatsnew per locale and/or have different content per locale, but it isn't a blocker for turning /whatsnew back on in August.
(Reporter)

Comment 4

5 years ago
(In reply to Jennifer Bertsch [:jbertsch] from comment #3)
> different
> content per locale,

FYI: This part is 100% handled by webdev, so we won't be addressing that here.

> Ideally, we would like to turn on /whatsnew per locale
> but it isn't a blocker for turning /whatsnew back on in
> August.

OK, that's good to know. Given that, I think we should figure out how to do this per-locale as part of this bug. I don't think it makes sense to add a global toggle now just to replace it with a per-locale toggle later.

This bug doesn't block turning it on in August, though. We can do that by hand as long as we're informed prior to the release starting.
Product: mozilla.org → Release Engineering
(Reporter)

Updated

5 years ago
OS: Linux → All
Priority: P3 → --
Summary: make it possible to control whatsnew page showing or not via ship it → make it possible to control whatsnew page showing or not via ship it, per-locale

Comment 5

5 years ago
Tracking for FF25, since we want to do this in the 25 timeframe.
tracking-firefox25: --- → +
(Assignee)

Updated

5 years ago
Depends on: 907404
(Assignee)

Comment 6

5 years ago
Bug 907404 is going to make actions=silent the in-app default. To get a whatsnew page will need to set this in the appropriate snippets:
   actions="showURL"  openURL="http://blah.com/"
 
It looks from the code at 
 http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#568
that the url provided will not be interpolated at runtime, so will have take care of expanding %LOCALE% and %VERSION% in
  https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/whatsnew/?oldversion=%OLD_VERSION%
when generating the snippets.

bhearsum and I talked about implementation a bit. He said he could probably do the release-runner side of this mid-september, and I volunteered to take care of buildbot/patcher-config bumper/snippet generation.
Assignee: nobody → nthomas
Priority: -- → P2
(Assignee)

Comment 7

5 years ago
(In reply to Nick Thomas [:nthomas] from comment #6)
> Bug 907404 is going to make actions=silent the in-app default. To get a
> whatsnew page will need to set this in the appropriate snippets:
>    actions="showURL"  openURL="http://blah.com/"

Oops, that's the update.xml. The snippets will have 
 actions=showURL
 openURL=http://blah.com/

Updated

4 years ago
Blocks: 927975
Hi Nick and Ben-

I just wanted to check in to see if this fix is still on track for Fx 25 on Oct 29?

If yes, we'll ask to proceed with https://bugzilla.mozilla.org/show_bug.cgi?id=927975

Thanks,
Jen

Comment 9

4 years ago
Just emailed Nick to find out whether we need a one-off update for pl, since it doesn't appear this will be completed by next Tuesday.
(Reporter)

Comment 10

4 years ago
(In reply to Alex Keybl [:akeybl] from comment #9)
> Just emailed Nick to find out whether we need a one-off update for pl, since
> it doesn't appear this will be completed by next Tuesday.

Yeah, we didn't get to this, unfortunately :(. We'd need it before we start the 25.0 builds anyways, which is today.

However, we can set pl to show the whatsnew page by hand. I'll make sure the person taking care of 25.0 is in the loop on what needs to be done.

Apologies for not getting to this.
(Reporter)

Updated

4 years ago
No longer blocks: 927975
(Reporter)

Comment 11

4 years ago
Per comment #7, we need to modify the "actions" section of the snippets and add "openURL". This should do the trick:
for snippet in `find Firefox-25.0-build1 -path '*/pl/*'`; do
  sed -i -e 's/actions=silent/actions=showURL/' $snippet
  echo "openURL=https://www.mozilla.org/pl/firefox/25.0/whatsnew/?oldversion=%OLD_VERSION%" >> $snippet
done

Comment 12

4 years ago
(In reply to Ben Hearsum [:bhearsum] from comment #10)
> (In reply to Alex Keybl [:akeybl] from comment #9)
> > Just emailed Nick to find out whether we need a one-off update for pl, since
> > it doesn't appear this will be completed by next Tuesday.
> 
> Yeah, we didn't get to this, unfortunately :(. We'd need it before we start
> the 25.0 builds anyways, which is today.
> 
> However, we can set pl to show the whatsnew page by hand. I'll make sure the
> person taking care of 25.0 is in the loop on what needs to be done.
> 
> Apologies for not getting to this.

That's file for now as long as we can get /pl enabled. Abstracting that logic and making it easier to do in future is still the goal. Thanks again.

Updated

4 years ago
tracking-firefox25: + → ---
(In reply to Ben Hearsum [:bhearsum] from comment #11)
> Per comment #7, we need to modify the "actions" section of the snippets and
> add "openURL". This should do the trick:
> for snippet in `find Firefox-25.0-build1 -path '*/pl/*'`; do
>   sed -i -e 's/actions=silent/actions=showURL/' $snippet
>   echo
> "openURL=https://www.mozilla.org/pl/firefox/25.0/whatsnew/
> ?oldversion=%OLD_VERSION%" >> $snippet
> done

https://wiki.mozilla.org/Releases/Firefox_25.0/BuildNotes
(Assignee)

Comment 14

4 years ago
Created attachment 8391684 [details] [diff] [review]
[tools] WIP

Adds support for an action-locales parameter to the patcher config, which lets you set what locales an action applies to (or all if you leave it out). You get snippet diffs like this:

--- reference/aus2/Firefox/26.0/Darwin_x86-gcc3-u-i386-x86_64/20131205075310/en-US/release/complete.txt 2014-03-14 21:43:08.000000000 +1300
+++ aus2/Firefox/26.0/Darwin_x86-gcc3-u-i386-x86_64/20131205075310/en-US/release/complete.txt   2014-03-14 22:42:06.000000000 +1300
@@ -11,2 +11,3 @@
 detailsUrl=https://www.mozilla.com/en-US/firefox/28.0/releasenotes/
-actions=silent
+actions=showURL
+openURL=https://www.mozilla.org/en-US/firefox/28.0/whatsnew/

which is what we wanted. But ...

diff -rU1 reference/aus2/Firefox/26.0/Darwin_x86_64-gcc3-u-i386-x86_64/20131205075310/ach/release/complete.txt aus2/Firefox/26.0/Darwin_x86_64-gcc3-u-i386-x86_64/20131205075310/ach/release/complete.txt
--- reference/aus2/Firefox/26.0/Darwin_x86_64-gcc3-u-i386-x86_64/20131205075310/ach/release/complete.txt        2014-03-14 21:43:08.000000000 +1300
+++ aus2/Firefox/26.0/Darwin_x86_64-gcc3-u-i386-x86_64/20131205075310/ach/release/complete.txt  2014-03-14 22:42:06.000000000 +1300
@@ -11,2 +11 @@
 detailsUrl=https://www.mozilla.com/ach/firefox/28.0/releasenotes/
-actions=silent

which we maybe don't. Silent became the default in Firefox 24.0 (via http://hg.mozilla.org/releases/mozilla-release/rev/d3736955cf31), so it might just work fine. Needs testing.

Patch needs some tests too.
(Assignee)

Comment 15

4 years ago
I took Firefox 23.0, and fed it the current update.xml but with actions="silent" removed. No page was opened after restart. That makes sense because startup.homepage_override_url would be blank then. As the pref goes back at least as far as Firefox 12 (the last update watershed) we're ok to remove silent from our configs and xml.
(Assignee)

Comment 16

4 years ago
Created attachment 8392129 [details] [diff] [review]
[tools] Add action-locale directive to patcher config

I added some tests since the WIP. The basic idea here is to read the optional schema 2 args if
* the locale is the list actions-locale
* or any locale if one isn't given
Attachment #8391684 - Attachment is obsolete: true
Attachment #8392129 - Flags: review?(bhearsum)
(Assignee)

Comment 17

4 years ago
re testing of snippets, I've took the beta config and truncated the last release to a locale list of 'de en-US ru', then tested
* no action-locales --> no change in snippets
* action-locales set to 'de en-US',
de snippets:
-actions=silent
+actions=showURL
+openURL=https://www.mozilla.org/de/firefox/29.0/whatsnew/

en-US snippets:
-actions=silent
+actions=showURL
+openURL=https://www.mozilla.org/en-US/firefox/29.0/whatsnew/

ru snippets:
-actions=silent

Also tested for just 'en-US' in action-locales, which works too (de like ru).
Blocks: 979599
(Reporter)

Comment 18

4 years ago
Comment on attachment 8392129 [details] [diff] [review]
[tools] Add action-locale directive to patcher config

Review of attachment 8392129 [details] [diff] [review]:
-----------------------------------------------------------------

I noticed that one of the existing tests (and your new one, probably because of copy/paste) wasn't getting run because of a typo "tes" vs. "test". The test fails ('actions' vs. ['actions']), so I'll fix that when I land this.

::: lib/python/release/updates/patcher.py
@@ +66,5 @@
> +                                                    self['release'][version]['locales']):
> +                for attr in SCHEMA_2_OPTIONAL_ATTRIBUTES:
> +                    if attr in self['current-update']:
> +                        attrs[attr] = substitutePath(self['current-update'][attr],
> +                                                     locale=locale)

This seems OK, but I think something needs renaming. The patcher config list is called "action-locales", but we've been calling things "attributes" in most other places. I'm not going to block on this, but I'd like to see the naming fixed up before this bug is closed.
Attachment #8392129 - Flags: review?(bhearsum) → review+
(Reporter)

Updated

4 years ago
Attachment #8392129 - Flags: checked-in+
(Assignee)

Comment 19

4 years ago
Good catch on the test. Do you mean that action-locales isn't a schema attribute like actions/openURL/etc ? I had been treating it like from/to/channel/testchannel - a directive to the snippet generator rather than something that goes in the snippets.
Whiteboard: [Australis:M-]
(Reporter)

Comment 20

4 years ago
(In reply to Nick Thomas [:nthomas] from comment #19)
> Good catch on the test. Do you mean that action-locales isn't a schema
> attribute like actions/openURL/etc ? I had been treating it like
> from/to/channel/testchannel - a directive to the snippet generator rather
> than something that goes in the snippets.

Ah, I see what you mean. Makes sense to me with that in mind. It's in line name-wise with things like "force", too.
(Assignee)

Comment 21

4 years ago
Ok, lets close this out.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.