739960 - provide method of updating root-level attributes of blobs

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

In order to allow the submission of release builds we need to provide a way of letting the automation update the root-level attributes of the blobs, things like detailsUrl, fileUrls, ftpFilenames, etc.

Comment 1

[[e-mail] (:bhearsum)]

https://github.com/bhearsum/balrog/compare/master...top-level-blob-api has a working version of this. I'm not super happy with it though, there's a lot of common code in releases.py that could use some refactoring.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: first pass

I tried to limit the scope of this patch, but in addition to the core of this bug some other small fixes/changes crept in. Here's the summary:
- configure logging in the wsgi apps to add timestamps
- minor improvements to a few logging messages
- support blob changes in Releases.updateRelease
- add Releases.localeExists, for easy testing of that in the views
- rename "details" in web-side code to "data", to be consistent with the DB (most of the necessary code changes here are in the tests)
- add auslib/util/retry.py to wrap the upstream retry with good defaults
- create a ReleaseForm and use it in existing code
- fix DbEditableForm to use an IntegerField, because HiddenField + NumberRange doesn't coerce the data to int
- fix JSONTextField to set process_errors instead of raising, because that's how Form.validate() likes it.
- add CSRF_ENABLED = False for tests, because we're using .validate() for the new ReleaseForm
- fix PermissionsView.get to render using the Form instead of the db rows
- add SingleReleaseView._post, for updating root-level blob attributes
- remove some unused imports

I'm not super happy with the views/releases.py still, but I'm not sure how to improve at this point, so maybe you folks can tell me whether it's really as gnarly as I think it is ;). The particular things I'm not happy with are:
- The opening block of both views does exactly the same thing. I don't know how to factor it out because of the return statement.
- The flow of both views is the same (loop over all releases, check if the thing exists, check product, update the version, do the work), but because of all the subtle differences I can't factor anything out. I'm starting to think that this kind of repetition is just a necessary evil.

Comment 3

[Erick Dransch [:edransch]]

Comment on attachment 612533 [details] [diff] [review]
first pass

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

::: auslib/web/views/forms.py
@@ +38,3 @@
>  
>  class DbEditableForm(Form):
> +    data_version = IntegerField('data_version', widget=HiddenInput())

Unless there is a good reason, we should try to keep the 'Required()' validator here. 
Is there a different way we can work around cases where we need optional data_version fields?

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Erick Dransch [:edransch] from comment #3)
> Comment on attachment 612533 [details] [diff] [review]
> first pass
> 
> Review of attachment 612533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: auslib/web/views/forms.py
> @@ +38,3 @@
> >  
> >  class DbEditableForm(Form):
> > +    data_version = IntegerField('data_version', widget=HiddenInput())
> 
> Unless there is a good reason, we should try to keep the 'Required()'
> validator here. 
> Is there a different way we can work around cases where we need optional
> data_version fields?

So far, the only case this has been necessary is for Releases views, because we do implicit creation of them. To solve this for now, I reverted this line and made ReleaseForm a subclass of Form, with its own, optional data_version. The Releases views do their own validation of data_version.

Comment 5

[Chris AtLee [:catlee]]

Comment on attachment 612533 [details] [diff] [review]
first pass

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

::: admin.wsgi
@@ -17,5 @@
>      for err in errors:
>          print >>sys.stderr, err
>      sys.exit(1)
>  
> -logging.basicConfig(filename=cfg.getLogfile(), level=cfg.getLogLevel())

do you want to include the log level in the output too? I often find that helpful when grepping through logs looking for errors when debugging is enabled.

::: auslib/db.py
@@ +60,5 @@
>      def __exit__(self, *exc):
>          try:
>              # If something that executed in the context raised an Exception,
>              # rollback and re-raise it.
> +            log.debug("AUSTransaction.__exit__: exc is:", exc_info=True)

FTW!

::: auslib/test/web/views/test_releases.py
@@ +29,5 @@
> +        }
> +    }
> +}
> +"""))
> +

curious why you swapped from self.assertEqual(json.loads(...), dict(...)) in test_db.py, and are adding the self.assertEqual(json.loads(...), json.loads(...)) style in here. Not a big deal, but would be nice to be consistent if there's no reason not to be.

Personally I find the json style easier to read. Could also be written as a python dict with {}.

::: auslib/web/templates/fragments/releases.html
@@ +12,5 @@
>      <tr>
> +        <td>{{ name }}</td>
> +        <td>{{ form.product()|safe }}</td>
> +        <td>{{ form.version()|safe }}</td>
> +        <td> <a href='releases/{{ name }}'>link</a></td>

does this need |safe, or to be url encoded?

::: auslib/web/views/releases.py
@@ +143,5 @@
> +        if new:
> +            return Response(status=201)
> +        else:
> +            return Response(status=200)
> +

I see what you mean by the similarities here. Maybe put the functions side-by-side and see where the differences are? Could you create a general function, and have both of these methods pass callbacks into it to do the method-specific work? Or have the general function be a decorator you apply?

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Chris AtLee [:catlee] from comment #5)
> > -logging.basicConfig(filename=cfg.getLogfile(), level=cfg.getLogLevel())
> 
> do you want to include the log level in the output too? I often find that
> helpful when grepping through logs looking for errors when debugging is
> enabled.

Good idea!

> ::: auslib/test/web/views/test_releases.py
> @@ +29,5 @@
> > +        }
> > +    }
> > +}
> > +"""))
> > +
> 
> curious why you swapped from self.assertEqual(json.loads(...), dict(...)) in
> test_db.py, and are adding the self.assertEqual(json.loads(...),
> json.loads(...)) style in here. Not a big deal, but would be nice to be
> consistent if there's no reason not to be.
> 
> Personally I find the json style easier to read. Could also be written as a
> python dict with {}.

Hmmm. I think what had happened here is that I changed it to compare JSON strings at one point, and then after hitting whitespace/ordering issues, changed it to dict()s. I agree that the json style is better though, I'll change these back.

> ::: auslib/web/templates/fragments/releases.html
> @@ +12,5 @@
> >      <tr>
> > +        <td>{{ name }}</td>
> > +        <td>{{ form.product()|safe }}</td>
> > +        <td>{{ form.version()|safe }}</td>
> > +        <td> <a href='releases/{{ name }}'>link</a></td>
> 
> does this need |safe, or to be url encoded?

They're definitely safe, good point.

> ::: auslib/web/views/releases.py
> @@ +143,5 @@
> > +        if new:
> > +            return Response(status=201)
> > +        else:
> > +            return Response(status=200)
> > +
> 
> I see what you mean by the similarities here. Maybe put the functions
> side-by-side and see where the differences are? Could you create a general
> function, and have both of these methods pass callbacks into it to do the
> method-specific work? Or have the general function be a decorator you apply?

I've done a bit of the side by side comparisons but couldn't figure out quite how to do it....

Callbacks might be an option, it makes me wonder if that ends up being even worse...I'll see if I can sketch it out a bit and see how it looks.

Comment 7

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #6)
> (In reply to Chris AtLee [:catlee] from comment #5)
> Callbacks might be an option, it makes me wonder if that ends up being even
> worse...I'll see if I can sketch it out a bit and see how it looks.

Ignoring the bad naming, here's what it looks like: https://github.com/bhearsum/balrog/blob/top-level-blob-api-callbacks/auslib/web/views/releases.py

Not as bad as I thought...

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #7)
> (In reply to Ben Hearsum [:bhearsum] from comment #6)
> > (In reply to Chris AtLee [:catlee] from comment #5)
> > Callbacks might be an option, it makes me wonder if that ends up being even
> > worse...I'll see if I can sketch it out a bit and see how it looks.
> 
> Ignoring the bad naming, here's what it looks like:
> https://github.com/bhearsum/balrog/blob/top-level-blob-api-callbacks/auslib/
> web/views/releases.py
> 
> Not as bad as I thought...

I did some more improvements to this and I'm actually pretty happy with it! Most notably, I adjusted a bunch of naming to be better/more consistent. I'm now:
- using 'data' to refer to any part of a Blob that isn't in a Blob object.
- using 'info' to refer to a full or partial row of the Releases table
- using 'blob' exclusively to refer to blob object instances.

And I added a docstring, too.

What do you all think about the attached patch vs. this github branch?

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 612533 [details] [diff] [review]
first pass

I've just reviewed the changes in the git repo. So far everything looks good. 

The only thing I have noticed while reviewing was my issue with understanding some variables, especially "data". It sounds too generic. Probably, not a big deal when you familiar with the code.

Comment 10

[[e-mail] (:bhearsum)]

(In reply to Rail Aliiev [:rail] from comment #9)
> Comment on attachment 612533 [details] [diff] [review]
> first pass
> 
> I've just reviewed the changes in the git repo. So far everything looks
> good. 
> 
> The only thing I have noticed while reviewing was my issue with
> understanding some variables, especially "data". It sounds too generic.
> Probably, not a big deal when you familiar with the code.

Yeah, outside of the context of a row in the Releases table, it's a tad generic. Personally, I'd rather be consistent even outside of that context, but I could be persuaded otherwise.

Comment 11

[[e-mail] (:bhearsum)]

Attachment: enable top level blob updates

This is pretty much the same as the github branch mentioned in comment #7, but there was a bunch of merging that had to happen, and I improved the names as noted in comment #8.

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 618639 [details] [diff] [review]
enable top level blob updates

Jenkins run is here: https://jenkins.mozilla.org/job/Balrog/88/

Comment 13

[[e-mail] (:bhearsum)]

Attachment: s/details/data/ in balrog client

I also renamed 'blob' variables to be 'data', to match the usage on the server (blob = a full release wrapped in a ReleaseBlob class, data = a full release or a subset of one, no wrapper).

Comment 14

[[e-mail] (:bhearsum)]

Comment on attachment 623652 [details] [diff] [review]
s/details/data/ in balrog client

Landed this, but it's still failing because of a missing csrf token. The client didn't really make this clear...it merely said:
"PUT /releases/Firefox-mozilla-central-nightly-20120513030522/builds/Linux_x86_64-gcc3/en-US HTTP/1.1" 400 None

After adding extra debug code on the server, I found this:
2012-05-14 06:33:46,579: errors are: {'csrf': ['Missing or invalid CSRF token']}

That error dictionary should be in the Response returned to the client, maybe we're not printing it properly?

Comment 15

[[e-mail] (:bhearsum)]

Reopening so we can finish fixing the client at some point.

Comment 16

[[e-mail] (:bhearsum)]

Attachment: add endpoint to retrieve a csrf token

This doesn't deal with returning a new token in the headers of POST/PUT/DELETE responses, I plan on dealing with that in bug 738000.

I ran all of Balrog through pyflakes, too, and this patch fixes a bunch of unused imports. The files containing actual changes are:
auslib/test/web/views/test_csrf.py
auslib/web/views/csrf.py

Comment 17

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #15)
> Reopening so we can finish fixing the client at some point.

Looks like bug 738000 is a better place to track that.