1109074 - Add a check on the tagging a release as shipped or not

Status: RESOLVED FIXED

(Release Engineering :: Applications: Shipit, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

In bug 1097661, we introduced the capability to tag if a release has been shipped or not.
However, it is easy to have both 34.0build1 and build2 tagged with status=postrelease
We should check and block that.

Comment 1

[Rail Aliiev [:rail]]

Can you elaborate on this? Do you mean mercurial tags? The patch in bug 1097661 doesn't look like it tags something...

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Sorry, I have been lazy in the description :)
By tag, I didn't mean an actual hg tag. It is just a way to update status to postrelease (to be used in the partial generation).

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1109074-Only-one-product-version-can-have-the-po.patch

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 8534385 [details] [diff] [review]
0001-Bug-1109074-Only-one-product-version-can-have-the-po.patch

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

::: kickoff/views/forms.py
@@ +152,5 @@
> +
> +            # Check if there is an other product-version already shipped
> +            similar = getReleases(status="postrelease", productFilter=release.product,
> +                                  versionFilter=release.version)
> +            if similar <> []:

This is probably better written as a simple "if similar", which will be True if similar is a non-empty list.

::: kickoff/views/releases.py
@@ +66,5 @@
> +
> +            if 'postrelease' in errors:
> +                # We are dealing with the ajax errors
> +                return Response(status=400, response=errors['postrelease'])
> +

This doesn't seem like a very stable assumption to make. We could easily lose error messages if we're modifying postrelease and something else at the same time. Is there a reason why the AJAX request can't cope with multiple error messages? Perhaps we should be returning JSON with key/value pairs instead of just values below?

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1109074-Only-one-product-version-can-have-the-po.patch

(In reply to Ben Hearsum [:bhearsum] from comment #4)


> ::: kickoff/views/releases.py
> @@ +66,5 @@
> > +
> > +            if 'postrelease' in errors:
> > +                # We are dealing with the ajax errors
> > +                return Response(status=400, response=errors['postrelease'])
> > +
> 
> This doesn't seem like a very stable assumption to make. We could easily
> lose error messages if we're modifying postrelease and something else at the
> same time. Is there a reason why the AJAX request can't cope with multiple
> error messages? Perhaps we should be returning JSON with key/value pairs
> instead of just values below?

Well, that is a good question! :)
Seems like errors.values() does not really work. It make the 
/releases/Firefox-28.0b2-build1
with :
INFO:werkzeug:127.0.0.1 - - [11/Dec/2014 16:56:17] "POST /releases/Firefox-28.0b2-build1 HTTP/1.1" 500 -
Traceback (most recent call last):
  File "/usr/share/pyshared/flask/app.py", line 1836, in __call__
    return self.wsgi_app(environ, start_response)
  File "/usr/share/pyshared/paste/auth/basic.py", line 95, in __call__
    return self.application(environ, start_response)
  File "/usr/share/pyshared/flask/app.py", line 1821, in wsgi_app
    return response(environ, start_response)
  File "/usr/lib/python2.7/dist-packages/werkzeug/wrappers.py", line 1206, in __call__
    app_iter, status, headers = self.get_wsgi_response(environ)
  File "/usr/lib/python2.7/dist-packages/werkzeug/wrappers.py", line 1194, in get_wsgi_response
    headers = self.get_wsgi_headers(environ)
  File "/usr/lib/python2.7/dist-packages/werkzeug/wrappers.py", line 1145, in get_wsgi_headers
    for x in self.response)
  File "/usr/lib/python2.7/dist-packages/werkzeug/wrappers.py", line 1145, in <genexpr>
    for x in self.response)
  File "/usr/lib/python2.7/dist-packages/werkzeug/_compat.py", line 106, in to_bytes
    raise TypeError('Expected bytes')
TypeError: Expected bytes


So, since I don't want to break the other behavior, that is why I did a special case...

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> Created attachment 8535027 [details] [diff] [review]
> 0001-Bug-1109074-Only-one-product-version-can-have-the-po.patch
> 
> (In reply to Ben Hearsum [:bhearsum] from comment #4)
> 
> 
> > ::: kickoff/views/releases.py
> > @@ +66,5 @@
> > > +
> > > +            if 'postrelease' in errors:
> > > +                # We are dealing with the ajax errors
> > > +                return Response(status=400, response=errors['postrelease'])
> > > +
> > 
> > This doesn't seem like a very stable assumption to make. We could easily
> > lose error messages if we're modifying postrelease and something else at the
> > same time. Is there a reason why the AJAX request can't cope with multiple
> > error messages? Perhaps we should be returning JSON with key/value pairs
> > instead of just values below?
> 
> Well, that is a good question! :)
> Seems like errors.values() does not really work. It make the 
> /releases/Firefox-28.0b2-build1
> with :
> INFO:werkzeug:127.0.0.1 - - [11/Dec/2014 16:56:17] "POST
> /releases/Firefox-28.0b2-build1 HTTP/1.1" 500 -
> Traceback (most recent call last):
>   File "/usr/share/pyshared/flask/app.py", line 1836, in __call__
>     return self.wsgi_app(environ, start_response)
>   File "/usr/share/pyshared/paste/auth/basic.py", line 95, in __call__
>     return self.application(environ, start_response)
>   File "/usr/share/pyshared/flask/app.py", line 1821, in wsgi_app
>     return response(environ, start_response)
>   File "/usr/lib/python2.7/dist-packages/werkzeug/wrappers.py", line 1206,
> in __call__
>     app_iter, status, headers = self.get_wsgi_response(environ)
>   File "/usr/lib/python2.7/dist-packages/werkzeug/wrappers.py", line 1194,
> in get_wsgi_response
>     headers = self.get_wsgi_headers(environ)
>   File "/usr/lib/python2.7/dist-packages/werkzeug/wrappers.py", line 1145,
> in get_wsgi_headers
>     for x in self.response)
>   File "/usr/lib/python2.7/dist-packages/werkzeug/wrappers.py", line 1145,
> in <genexpr>
>     for x in self.response)
>   File "/usr/lib/python2.7/dist-packages/werkzeug/_compat.py", line 106, in
> to_bytes
>     raise TypeError('Expected bytes')
> TypeError: Expected bytes
> 
> 
> So, since I don't want to break the other behavior, that is why I did a
> special case...

If it doesn't work, now is a good time to fix it :). AFAICT, the only place we hit that endpoint is https://github.com/mozilla/build-tools/blob/master/buildfarm/release/release-runner.py#L82, and all it does is print, so you should be safe to change the response in whatever way you need to there.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Attachment: 0001-Bug-1109074-Only-one-product-version-can-have-the-po.patch

Right, here it is :)

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8535090 [details] [diff] [review]
0001-Bug-1109074-Only-one-product-version-can-have-the-po.patch


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

Comment 9

[Sylvestre Ledru [:Sylvestre]]

fixed and deployed