721233 - API for manipulating balrog rules

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), defect, P3)

Comment 1

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

found in triage.

Comment 2

[[e-mail] (:bhearsum)]

GET/POST for /rules/:id got done as part of bug 734398. The only thing left to do here is DELETE support, which is less critical.

Comment 3

[[e-mail] (:bhearsum)]

mass component change

Comment 4

[[e-mail] (:bhearsum)]

We need a /rules GET implementation too, because anyone updating a rule via the API needs its id. Bonus points if you can pass various things to filter the rules.

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #2)
> GET/POST for /rules/:id got done as part of bug 734398. The only thing left
> to do here is DELETE support, which is less critical.

POST is done, but doesn't support incremental updates. Eg, "only change the mapping for rule X". If you omit fields from the POST right now, they'll get set to NULL.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: allow partial rule updates

This isn't particularly pretty, but it does the job. We can't simply iterate over a form's fields for a couple of reasons:
* wtforms doesn't give us a way to know whether the field was passed in the POST
* our field names don't line up perfectly with our column names, so we'd end up trying to set things like build_id (instead of buildID) when updating the rule.

Comment 7

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

Comment on attachment 8371700 [details] [diff] [review]
allow partial rule updates

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

Apologies for the delay in looking at this. Just a little test tidying up to do.

::: auslib/admin/views/forms.py
@@ +113,5 @@
>      comment = NullableTextField('Comment', validators=[validators.Length(0,500) ])
>      update_type = SelectField('Update Type', choices=[('minor','minor'), ('major', 'major')], validators=[])
>      header_arch = NullableTextField('Header Architecture', validators=[validators.Length(0,10) ])
>  
> +class EditRuleForm(DbEditableForm):

Not wild about the duplication here, but you probably already looked at continuing to subclass RuleForm and modifying the validators.

@@ +122,5 @@
> +    version = NullableTextField('Version', validators=[Optional(), validators.Length(0,10) ])
> +    build_id = NullableTextField('BuildID', validators=[Optional(), validators.Length(0,20) ])
> +    channel = NullableTextField('Channel', validators=[Optional(), validators.Length(0,75) ])
> +    locale = NullableTextField('Locale', validators=[Optional(), validators.Length(0,10) ])
> +    distribution = NullableTextField('Distrubution', validators=[Optional(), validators.Length(0,100) ])

Typo 'Distrubution' here and in the RuleForm definition.

::: auslib/test/admin/views/test_rules.py
@@ +49,5 @@
>          self.assertEquals(ret.status_code, 200)
>          self.assertTrue("c" in ret.data, msg=ret.data)
>  
>  
> +class TestSingleRuleView_JSON(ViewTest, JSONTestMixin):

There's no check on content of the json response here, so I'm confused what this is testing.

@@ +60,5 @@
> +        # Check that what we requested was updated.
> +        self.assertEquals(r[0]['mapping'], 'a')
> +        # And check that another part of the rule hasn't changed.
> +        self.assertEquals(r[0]['backgroundRate'], 100)
> +

TestSingleRuleView_HTML is already checking rule changes, but has been passing because it wasn't checking that the unmodified parts of the rule were left alone. Lets move or duplicate that check there.

Comment 8

[[e-mail] (:bhearsum)]

Attachment: adjust tests; fix SelectField bug

(In reply to Nick Thomas [:nthomas] from comment #7)
> Comment on attachment 8371700 [details] [diff] [review]
> allow partial rule updates
> 
> Review of attachment 8371700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Apologies for the delay in looking at this. Just a little test tidying up to
> do.
> 
> ::: auslib/admin/views/forms.py
> @@ +113,5 @@
> >      comment = NullableTextField('Comment', validators=[validators.Length(0,500) ])
> >      update_type = SelectField('Update Type', choices=[('minor','minor'), ('major', 'major')], validators=[])
> >      header_arch = NullableTextField('Header Architecture', validators=[validators.Length(0,10) ])
> >  
> > +class EditRuleForm(DbEditableForm):
> 
> Not wild about the duplication here, but you probably already looked at
> continuing to subclass RuleForm and modifying the validators.

Yeah, it's surprisingly difficult to modify an existing form. I couldn't even find a reasonable way to iterate over fields =(. As a metapoint, I'm somewhat regretting using WTForms at all after seeing how much subclassing/extending we've had to do to implement what IMO are fairly basic things.

> @@ +122,5 @@
> > +    version = NullableTextField('Version', validators=[Optional(), validators.Length(0,10) ])
> > +    build_id = NullableTextField('BuildID', validators=[Optional(), validators.Length(0,20) ])
> > +    channel = NullableTextField('Channel', validators=[Optional(), validators.Length(0,75) ])
> > +    locale = NullableTextField('Locale', validators=[Optional(), validators.Length(0,10) ])
> > +    distribution = NullableTextField('Distrubution', validators=[Optional(), validators.Length(0,100) ])
> 
> Typo 'Distrubution' here and in the RuleForm definition.

Fixed.

> @@ +60,5 @@
> > +        # Check that what we requested was updated.
> > +        self.assertEquals(r[0]['mapping'], 'a')
> > +        # And check that another part of the rule hasn't changed.
> > +        self.assertEquals(r[0]['backgroundRate'], 100)
> > +
> 
> TestSingleRuleView_HTML is already checking rule changes, but has been
> passing because it wasn't checking that the unmodified parts of the rule
> were left alone. Lets move or duplicate that check there.

Good point, I got rid of this and did better checking in the existing test. I also uncovered a problem with SelectField's, where you end up with u'None' instead of None when you don't provide them in the POST request. Luckily, WTForms has a fairly simple way to let us fix that.

Comment 9

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

Comment on attachment 8376320 [details] [diff] [review]
adjust tests; fix SelectField bug

lgtm

Comment 10

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/1747ff269760222ff723832818cb7b9fa899e405
bug 721233: API for manipulating balrog rules - support partial updates of rules. r=nthomas

Comment 11

[[e-mail] (:bhearsum)]

Comment on attachment 8376320 [details] [diff] [review]
adjust tests; fix SelectField bug

Landed, and I'm planning to do a push to Balrog prod this week.

Comment 12

[[e-mail] (:bhearsum)]

Deployed to prod.