API for manipulating balrog rules

RESOLVED FIXED

Status

Release Engineering
Balrog: Backend
P3
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bhearsum, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
found in triage.
Component: Release Engineering → Release Engineering: Automation
QA Contact: release → catlee
(Assignee)

Comment 2

5 years ago
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.
Product: mozilla.org → Release Engineering
(Assignee)

Comment 3

4 years ago
mass component change
Component: General Automation → Balrog: Backend
(Assignee)

Updated

4 years ago
Blocks: 933414
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 739959
(Assignee)

Comment 5

3 years ago
(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.
Assignee: nobody → bhearsum
(Assignee)

Comment 6

3 years ago
Created attachment 8371700 [details] [diff] [review]
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.
Attachment #8371700 - Flags: review?(nthomas)
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.
Attachment #8371700 - Flags: review?(nthomas) → review-
(Assignee)

Comment 8

3 years ago
Created attachment 8376320 [details] [diff] [review]
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.
Attachment #8371700 - Attachment is obsolete: true
Attachment #8376320 - Flags: review?(nthomas)
Comment on attachment 8376320 [details] [diff] [review]
adjust tests; fix SelectField bug

lgtm
Attachment #8376320 - Flags: review?(nthomas) → review+

Comment 10

3 years ago
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
(Assignee)

Comment 11

3 years ago
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.
Attachment #8376320 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Depends on: 976081
(Assignee)

Comment 12

3 years ago
Deployed to prod.
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.