1049689 - Automatic filling of the partials field in ship-it

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Autofill-the-partials-field.patch

In ship-it, the partials field has to be provided by the user. 

AFAIK, in most of the cases, this can be guess by the version. So, let's delegate this to the machine.

I applied the same rules for Firefox and Thunderbird. Fennec is not impacted (no partial).

Here are the rules implemented:
beta 1 => ver-1 : RC + beta 10
beta 2 => beta 1  + ver-1 + RC
beta 3..10 => beta-1 / beta-2

Release: ver-3, ver-2 and ver-1

Notes:
* The code has no knowledge of previous builds. That means that if the release is 29.0.1, the code will still propose 29.0build1 as partial.
* Idem, if we have a 31.0b2build2, the code will propose 31.0b2build1


I know it is not perfect but this is still better than the current state. Don't hesitate if you have a suggestion for a better implementation.

The attached patch proposes an implementation of this.

The field remains editable in case of bad guess.

This bug depends on 1048290 (the patches have to be applied on this order).

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Writing this bug, I wonder if I should not use the same method as the current suggestion to generate a list of previous build to improve the data.

Like:
    setupVersionSuggestions(
        $('#firefox-version'),
        ["30.0b1", "29.0b3", "29.0b2"],

I will give it a try but Ben, I would be happy to get your input on the patch or this idea.

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Driveby comments ...

(In reply to Sylvestre Ledru [:sylvestre] from comment #0)
> Here are the rules implemented:
> beta 1 => ver-1 : RC + beta 10

There's no beta 10 these days, and we can't put any RC builds into the partial list until the changes in bug 908134 are done. But after that I'd suggest the last two of the build sequence [ b8 b9 rc1 rc2 ... ]. 

Perhaps we can build a list of previous releases by looking at the product name and branch. beta would need m-b and m-r, except for any y.0.x releases.

> beta 2 => beta 1  + ver-1 + RC

See above with b1 appended to list.

> beta 3..10 => beta-1 / beta-2

beta 3..9 => beta-1 + beta-2

> Release: ver-3, ver-2 and ver-1

I think we should still check the ADI for these, it may not always be the right choice. Eg 29.0 vs 29.0.1, and more on 28.0 than 29.0.

Thunderbird does something different again, check shipit but I think it's just do the last beta/release for them.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8468557 [details] [diff] [review]
0001-Autofill-the-partials-field.patch

Actually, I am not happy about my change. I will try something else.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Autofill-the-partials-field.patch

I was not happy about the previous solution. Too much strings and no knowledge about build numbers.

Instead, I am using the partials data already generated. Instead of 7 weeks, I updated the number to 15 weeks to get the two last releases.

For the release, I am using the last two partials. Beta, the 3 last partials.

Comment 5

[[e-mail] (:bhearsum)]

Most of comment #2 sounds fine to me, but I'm worried about a couple of things where we have more than one build number for a release:
* What will be autofilled? My read makes me think that we'll get eg, 32.0b3build1 and 32.0b3build2 as partials.
* What happens when we built build1 and build2, but shipped build1? I don't see any way for the system to make a safe assumption about this until bug 886522 is fixed.

Going to avoid commenting on the code for now.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

(In reply to Ben Hearsum [:bhearsum] from comment #5)
> Most of comment #2 sounds fine to me, but I'm worried about a couple of
> things where we have more than one build number for a release:
> * What will be autofilled? My read makes me think that we'll get eg,
> 32.0b3build1 and 32.0b3build2 as partials.
No, the current system returns only 32.0b3build2 in this case (I haven't touch that, the values are provided by the Python engine).

> * What happens when we built build1 and build2, but shipped build1? I don't
> see any way for the system to make a safe assumption about this until bug
> 886522 is fixed.
The current suggestion system will also propose build2. Here, it is our (release management) duty to check that. My proposal allows further changes (and it is still better than the current state).

Comment 7

[[e-mail] (:bhearsum)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> (In reply to Ben Hearsum [:bhearsum] from comment #5)
> > Most of comment #2 sounds fine to me, but I'm worried about a couple of
> > things where we have more than one build number for a release:
> > * What will be autofilled? My read makes me think that we'll get eg,
> > 32.0b3build1 and 32.0b3build2 as partials.
> No, the current system returns only 32.0b3build2 in this case (I haven't
> touch that, the values are provided by the Python engine).

OK, I forgot about that. This is more of a footgun here though...if we get the version or build number wrong, it fails very early. If we get the partials wrong, we may not now for a whole day. Fixing it requires a full rebuild, and the alternative is to go without partials for that version. I know it's not me that's generally interacting with Ship It, but I'm having trouble convincing myself that something that isn't bulletproof is good enough here. I'm not trying to be stop energy or wontfix this bug, but I do have concerns.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

(In reply to Ben Hearsum [:bhearsum] from comment #7)
> OK, I forgot about that. This is more of a footgun here though...
Yes but my changes just improve the usability. It does not add (or decrease) the risks here.
If you want, I can add a warning message with something like "please check the value as it is 95.2% accurate" ;).

What about extending the release runner sanity checks to perform the checks?

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Autofill-the-partials-field.patch

Comment 10

[Sylvestre Ledru [:Sylvestre]]

patch refreshed

Comment 11

[[e-mail] (:bhearsum)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> (In reply to Ben Hearsum [:bhearsum] from comment #7)
> > OK, I forgot about that. This is more of a footgun here though...
> Yes but my changes just improve the usability. It does not add (or decrease)
> the risks here.
> If you want, I can add a warning message with something like "please check
> the value as it is 95.2% accurate" ;).
> 
> What about extending the release runner sanity checks to perform the checks?

The problem right now is that there is no programatic way to determine this, even from release runner. We don't store "we shipped build X" anywhere.

I said before that we need bug 886522 for this, but I think we can just wait for bug 826753 instead. Because of the way John is implementing it, we should have that information when his work is done. I strongly feel that we should wait for that. I don't want to add a footgun that can cause huge delays to releases.

We also need to come to some sort of agreement on the partials for the release channel. I share Nick's concerns there.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Looks like bug 826753 is on track for Q4. Let's revisit this once bug 826753 ships.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1049689-Autofill-the-partials-field.patch

rebase of the patch

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1049689-Autofill-the-partials-field.-We-are-prop.patch

Because we are now able to mark if a release has been shipped or not (bug 1097661), this patch makes sense now.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0002-Bug-1049689-Autofill-the-partials-field.-We-are-prop.patch

Fix a typo

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 8532599 [details] [diff] [review]
0002-Bug-1049689-Autofill-the-partials-field.-We-are-prop.patch

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

::: kickoff/static/suggestions.js
@@ +52,1 @@
>      }

This special case could get interesting at times. We had to add a fake branch to ship 34.0.5, for example. I'm not going to block on this, but I'd like to see this special case removed at some point.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8532599 [details] [diff] [review]
0002-Bug-1049689-Autofill-the-partials-field.-We-are-prop.patch

merged:
http://git.mozilla.org/?p=build/release-kickoff.git;a=commit;h=f48eb516e148441809ae3d612667ad00b292dd53

Comment 18

[[e-mail] (:bhearsum)]

I pushed this to prod this morning.

Comment 19

[[e-mail] (:bhearsum)]

Firefox 35.0b3 shipped with a not ideal selection of partials (35.0b1build1, 34.0b9build1), did ship it suggest the wrong things here? 35.0b2build1 and 35.0b1build1 were the more logical choices.

Comment 20

[Nick Thomas [:nthomas] (UTC+12)]

From what I can tell, ship-it made a bad suggestion because not all recent betas had been marked as shipped. I've gone through and finished that off now (for all apps/channels), and added a manual step to the checklist to fix the process going forward. Bug 1111410 to automate, duped back to zeller's bug on enhancing ship-it.