Dashboard check automatically managed by ship-it

RESOLVED FIXED

Status

Release Engineering
Release Automation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [shipit])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8475142 [details] [diff] [review]
0001-Dashboard-check-automatically-managed.patch

The proposed patch managed the dashboard check transparently.

It is a beta => enable it
It is a release (or other cases) => disable it
Attachment #8475142 - Flags: review?(rail)
Attachment #8475142 - Flags: review?(bhearsum)
Comment on attachment 8475142 [details] [diff] [review]
0001-Dashboard-check-automatically-managed.patch

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

I'm really unhappy that this patch needs to exist at all. The only reason this option exists is because the l10n dashboard doesn't support the use cases we need it to. I'd much rather effort be spent fixing it than working around it even more.

::: kickoff/model.py
@@ +12,5 @@
>  
> +def isBeta(version):
> +    if re.match('.*b[0-9]', version):
> +        return True
> +    return False

I would prefer to see ANY_VERSION_REGEX being used here rather than a custom regex. It's a much more well tested and used regex. You can check its groups to detect a beta:
>>> from mozilla.build.versions import ANY_VERSION_REGEX
>>> import re
>>> a = re.match(ANY_VERSION_REGEX, "32.0b8")
>>> a
<_sre.SRE_Match object at 0x7ff657c6add8>
>>> a.groups()
('32.0', 'b8', 'b', None, None)

::: kickoff/templates/includes/fennec_release.html
@@ -50,5 @@
> -<div class="submit_release">
> -    {{ fennecForm.dashboardCheck.label()|safe }}
> -    {{ fennecForm.dashboardCheck()|safe }}
> -    <div class="help">Whether or not to ensure that the changesets given match the ones on the <a href="https://l10n.mozilla.org/shipping/milestones">L10n Dashboard</a>. This is normally turned on for betas and turned off for all other releases.</div>
> -</div>

I think it's a bad idea to remove this override. If something comes up where the the l10n dashboard is broken, you won't be able to kick off betas without releng manual work. I would suggest moving this to the Advanced Options section instead.
(Assignee)

Comment 2

3 years ago
Thanks for the feedback.

After chatting on IRC with Ben:
A better plan would be to move it to advanced, document it a bit more and update the javascript to check the box when we deal with a beta.
Attachment #8475142 - Attachment is obsolete: true
Attachment #8475142 - Flags: review?(rail)
Attachment #8475142 - Flags: review?(bhearsum)
(Assignee)

Comment 3

3 years ago
Created attachment 8475739 [details] [diff] [review]
0001-Better-management-of-the-dashboard-check.patch

Better management of the dashboard check:
 * Moved to advanced options
 * When building a beta release, tick the checkbox (not in other cases)
 * Improve a little the documentation
    
I took the liberty to do a minor refactoring in the same patch.
Attachment #8475739 - Flags: review?(bhearsum)
Attachment #8475739 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 4

3 years ago
Thanks. Pushed:
http://git.mozilla.org/?p=build/release-kickoff.git;a=commit;h=1cec9deec611df9e6ea239ced93b8e6fdb532f4a
(Assignee)

Updated

3 years ago
Assignee: nobody → sledru
(Assignee)

Comment 5

3 years ago
Deployed and working.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.