1021022 - support comparison operators for matching version & buildID in rules

Status: RESOLVED FIXED

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

Description

[[e-mail] (:bhearsum)]

We're going to need this for h264, and we'll probably need it to deal with watershed updates for Gecko products before we switch betas to Balrog

Comment 1

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

We already have regex matching for version in rules, and MozillaVersion as a subclass of distutils.version.StrictVersion to handle our naming, so not a lot of heavy lifting to do here.

Comment 2

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

Attachment: WIP

This still needs a lot more tests adding, and fixing up the test-rules.py cases which use wildcard version matching, but it's a good time for feedback. You may also know of a more pythonic way of doing this.

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 8461428 [details] [diff] [review]
WIP

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

(In reply to Nick Thomas [:nthomas] from comment #2)
> This still needs a lot more tests adding, and fixing up the test-rules.py
> cases which use wildcard version matching, but it's a good time for
> feedback. You may also know of a more pythonic way of doing this.

I've been muddling about this and the best I can come up using eval(). This would be pretty slick in string comparison, but you'd have to inject MozillaVersion(...) calls into the version strings. It doesn't _seem_ too unsafe, because all of the data comes through the admin interface. It's still a massive red flag though...

The patch looks fine overall, though like you say - needs more tests!

Comment 4

[[e-mail] (:bhearsum)]

I asked catlee about making this more pythonic without eval. He had an idea:
import operator
operators = {
    '>=': operator.ge,
    '>':  operator.gt,
    ...
}

def get_op(pattern):
    """Returns the operator and operand from a string pattern. e.g.

    >>> get_op('>=5')
    ('>=', '5')

    """
    # regexes!


def string_compare(value, compstr):
    """Returns True if value is True when compared against compstr. e.g.

    >>> string_compare("b", ">a")
    True
    """
    op, operand = get_op(compstr)
    opfunc = operators[op]
    return opfunc(value, operand)

def version_compare(value, compstr):
    """Returns True if value is True when compared against compstr and both are treated as versions. e.g.
    >>> version_compare("21.0b1", "<22.0")
    True
    """
    op, operand = get_op(compstr)
    opfunc = operators[op]
    value = MozillaVersion(value)
    operand = MozillaVersion(operand)
    return opfunc(value, operand)


And the IRC chatter:
10:03 <@catlee> bhearsum: yeah, I have an idea
10:03 <@catlee> bhearsum: also there's a typo in that code
10:03 <@catlee> return value <= pathern[2:]
10:03 -!- _6a68 [[e-mail]] has joined #releng
10:07 -!- pmoore|brb is now known as pmoore
10:08 <@catlee> bhearsum: https://python.pastebin.mozilla.org/5601146 ?
10:08 <@catlee> I don't see a reason for those to be instance methods
10:08 <@catlee> also I found the argument order a bit confusing...
10:09 <@catlee> e.g. you want the operator "in the middle"
10:10 < bhearsum> ahhhhh
10:10 < bhearsum> that operators dispatch table is very helpful
10:11 < bhearsum> mind if i paste this into the bug?
10:11 <@catlee> sure
10:11 < bhearsum> thanks!
10:11 <@catlee> probably results in same # of lines
10:12 < bhearsum> might be a few less because it factors out the operator parsing (which happens twice)
10:12 < bhearsum> in any case, may be more readable
10:13 <@catlee> yeah, and less error prone I think
10:13 <@catlee> e.g. we would have hit an error once we introduced a '>=' rule
10:13 <@catlee> <= sorry
10:13 < bhearsum> ahhh, that's a good point too
10:14 < bhearsum> easier to test these generic methods, too



I'm still okay with either, but the point about being less error prone stands out to me.

Comment 5

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

Attachment: Main patch

Notes
* the main part of patch is in AUS.py and util/comparisons.py (new)
* the ordering change in the rule lookup is to mimic how we think about things (slight perf win to discard a lot of rules quickly), otherwise unlrelated
* test_db.py needs some version changes to cast to MozillaVersion
* lots of new tests (over-testing?)
* removed versions from rules in aus-data-snapshots, not part of test except for version-globbing case. The later looks horrific but is a straight rename, except for rules.sql

We may need some error handling around the casting to MozillaVersion in version_compare(), and I'm looking at some validation in RuleForm in a followup patch.

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 8463312 [details] [diff] [review]
Main patch

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

(In reply to Nick Thomas [:nthomas] from comment #5)
> * lots of new tests (over-testing?)

I think it's just the right amount of tests. If they took a long time to run I might suggest cutting a few out, but they're basically a zero runtime, so it's good to have them I think.

> We may need some error handling around the casting to MozillaVersion in
> version_compare(),

I wonder if it's easier to deal with this by requiring a MozillaVersion to be passed in. Haven't really thought much about it, just an idea...

> and I'm looking at some validation in RuleForm in a
> followup patch.

This would be great!

Comment 7

[[github robot]]

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

https://github.com/mozilla/balrog/commit/32434c35474470e155fc5de518a26214b2016e7e
Bug 1021022, support comparison operators for matching version & buildID in rules, r=bhearsum

Comment 8

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

Attachment: Validation WIP

Still missing some tests for the update case, but interested in feedback at this point.

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 8490553 [details] [diff] [review]
Validation WIP

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

::: auslib/admin/views/forms.py
@@ +91,5 @@
> +        log.debug('starting in _validator, check_version is %s' % check_version)
> +
> +        # empty input is fine
> +        if field.data is None:
> +            return

Should this check by against None, or just a "not field.data"? I can never remember what the right here is....

You might want to try capturing a request made through rules.html - it's possible that version="" is set for those requests.

@@ +100,5 @@
> +        except TypeError:
> +            # get_op field returns None if no operator or no match, can't be unpacked
> +            raise validators.ValidationError('Invalid input for %s' % field.label)
> +
> +        if check_version:

It looks to me like this is a completely separate check from the operator validation. I can see how it's easy to do here, because you've already stripped the operator away, but it might be cleaner to do it as its own validator. I'm OK either way though.

Comment 10

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

I'm going to resolve this as the main code landed long ago, with bug 1233621 filed to add some server-side validation when rules are submitted via the API.