1257298 - support a list of values in rules' version field

Status: RESOLVED FIXED

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

Description

[[e-mail] (:bhearsum)]

Balrog's current version comparison logic allows us to do exact version matches, <, <=, >, and >=. This supports most of our use cases, but recently we've found some cases where we want to match all versions within one major version. Eg: 40.0, 40.0.1, 40.0.2, etc. The workaround for this right now is to add two rules, eg:
* Priority: 100, Version: < 40.0, Mapping: Old Thing
* Priority: 90, Version < 41.0, Mapping: New Thing

Mark suggested that maybe we could support globbing in versions, which would let us replace the above with a rule like:
* Priority: 100, Version: 40.*, Mapping: New Thing

This would probably work, but it might be a bit of a pain to make the globs work with our MozillaVersion parser. An alternative might be to allow multiple operators per version column, and use a rule like:
* Priority: 100, Version: 40.0 < {} < 41.0, Mapping: New Thing (where {} gets replaced with the incoming version)
or
* Priority: 100, Version: >40.0; <41.0, Mapping: New Thing (iterate over the comparsions)

I'm not quite as high on this idea now that I've written it out...it's uglier than I thought it might be.

Comment 1

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

X.* would be nice, but I agree it's not obvious how to modify the version parsing code in a clean way. We could have a copy of ModernMozillaVersion which allows * after the first '.', and implements a __cmp__() to handle that. Might not be easy to make sure that's only used for rules and not for updateQuery's though.

Another possibility is to allow a list of values, eg 
* * Priority: 100; Version: 40.0,40.0.1,40.0.2; Mapping: New Thing

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas (UTC+13) [:nthomas] from comment #1)
> X.* would be nice, but I agree it's not obvious how to modify the version
> parsing code in a clean way. We could have a copy of ModernMozillaVersion
> which allows * after the first '.', and implements a __cmp__() to handle
> that. Might not be easy to make sure that's only used for rules and not for
> updateQuery's though.

That's a really important point - we definitely wouldn't want to be doing any parsing on updateQuery versions. I think we have divergent use cases for ModernMozillaVersion if we go this route. Currently, we use it to do parsing and comparsions of real versions. Adding globs means we need some way to compare a real version against a globbed version. Modifying ModernMozillaVersion may be one way to do that, but we could also consider using a separate object to parse updateQuery objects, or maybe implementing globbing as an additional operator in https://github.com/mozilla/balrog/blob/master/auslib/util/comparison.py. The latter would require changing that code to support operators on the right hand side of the string, though.

> Another possibility is to allow a list of values, eg 
> * * Priority: 100; Version: 40.0,40.0.1,40.0.2; Mapping: New Thing

If we do CSV support, we'll need to make the column bigger, too (it's only 10 characters at the moment).

The more I think about this, the more I lean towards using CSV. It's actually more powerful than globbing (you can limit yourself to only certain point releases), and unless we get to a point where we've got tens of point releases per major version, using a short list instead of a glob shouldn't be a big deal. Let's start with this, and if we still want more version power afterwards, we can follow-up.

Comment 3

[A.S.]

While working on adding CSV support , I found a potential bug. For example : say the rules.version column contains a CSV value say " >=40.0, <40.0" . This will return true regardless of updateQuery input version. This issue needs to be taken care of too in the update patch.

I need to gather couple of use-case requirements before proceeding : 

1. will the version (input update query or in data store) ever contain an alphabet (as it did in OldMozilla version) ? Even though I can get a *.*.* mozilla version from old version but just wanted to be sure.

2. Since operators are present in version numbers, so I'm assuming that the following cases are possible for range of version numbers : (a) version number range : "<40.0" , (b) range: ">40.0" , (c) range : "<40.0 , <50.0" (i.e. ultimately it means <50.0) , (d) version number range : ">40.0, >50.0" i.e. ultimately ">40.0" .  (e) range : "<40.0 , >=50.0"

Cases (a) - (d) are simple as at most one-same operator is present in all the version numbers. i.e. a continuous range. However, case (e) is where there is a break in range. (0,40) Union [50,inf). 
Can case (e) ever occur ?

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Ashish Sareen from comment #3)
> While working on adding CSV support , I found a potential bug. For example :
> say the rules.version column contains a CSV value say " >=40.0, <40.0" .
> This will return true regardless of updateQuery input version. This issue
> needs to be taken care of too in the update patch.
> 
> I need to gather couple of use-case requirements before proceeding : 
> 
> 1. will the version (input update query or in data store) ever contain an
> alphabet (as it did in OldMozilla version) ? Even though I can get a *.*.*
> mozilla version from old version but just wanted to be sure.

It could, yes.

> 2. Since operators are present in version numbers, so I'm assuming that the
> following cases are possible for range of version numbers : (a) version
> number range : "<40.0" , (b) range: ">40.0" , (c) range : "<40.0 , <50.0"
> (i.e. ultimately it means <50.0) , (d) version number range : ">40.0, >50.0"
> i.e. ultimately ">40.0" .  (e) range : "<40.0 , >=50.0"
> 
> Cases (a) - (d) are simple as at most one-same operator is present in all
> the version numbers. i.e. a continuous range. However, case (e) is where
> there is a break in range. (0,40) Union [50,inf). 
> Can case (e) ever occur ?

I can't think of a valid use case for that. The more I think about this bug the more I think we should disallow operators when using a list of values - I haven't seen an example where allowing them would make things clearer. The most obvious use case we have for lists of values is when you want to match something like 40.0-40.0.5. Without operators, that looks like this:
40.0, 40.0.1, 40.0.2 ,40.0.3, 40.0.4, 40.0.5
With operators, we get:
>=40.0, <=40.0.5

To my eyes, the more verbose list is much easier to parse.

Comment 5

[[github robot]]

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

https://github.com/mozilla/balrog/commit/70ba3890129fc6e10d677154c17cffcb0bd730d1
Bug 1257298 - support a list of values in rules' version field (#217). r=bhearsum

Comment 6

[[e-mail] (:bhearsum)]

Ashish, I just realized that we forgot to make the "version" column bigger to accomodate this - can you follow-up with that? You'll need to modify db.py, and create a migration script similar to https://github.com/mozilla/balrog/blob/fdfd220bf3cddc54c4a8123b016344ef0cb862f0/auslib/migrate/versions/007_longer_locales.py.

Comment 7

[A.S.]

(In reply to Ben Hearsum (:bhearsum) from comment #6)
> Ashish, I just realized that we forgot to make the "version" column bigger
> to accomodate this - can you follow-up with that? You'll need to modify
> db.py, and create a migration script similar to
> https://github.com/mozilla/balrog/blob/
> fdfd220bf3cddc54c4a8123b016344ef0cb862f0/auslib/migrate/versions/
> 007_longer_locales.py.

How long should be rules.version field ?

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Ashish Sareen from comment #7)
> (In reply to Ben Hearsum (:bhearsum) from comment #6)
> > Ashish, I just realized that we forgot to make the "version" column bigger
> > to accomodate this - can you follow-up with that? You'll need to modify
> > db.py, and create a migration script similar to
> > https://github.com/mozilla/balrog/blob/
> > fdfd220bf3cddc54c4a8123b016344ef0cb862f0/auslib/migrate/versions/
> > 007_longer_locales.py.
> 
> How long should be rules.version field ?

We talked about this on IRC and decided we wanted to have enough room for ~10 versions.

Comment 9

[A.S.]

(In reply to Ben Hearsum (:bhearsum) from comment #8)
> (In reply to Ashish Sareen from comment #7)
> > (In reply to Ben Hearsum (:bhearsum) from comment #6)
> > > Ashish, I just realized that we forgot to make the "version" column bigger
> > > to accomodate this - can you follow-up with that? You'll need to modify
> > > db.py, and create a migration script similar to
> > > https://github.com/mozilla/balrog/blob/
> > > fdfd220bf3cddc54c4a8123b016344ef0cb862f0/auslib/migrate/versions/
> > > 007_longer_locales.py.
> > 
> > How long should be rules.version field ?
> 
> We talked about this on IRC and decided we wanted to have enough room for
> ~10 versions.

yeah. Posted here to keep a formal record. So decision made based on average 6 characters per version and need room for around 10 versions and 9 commas plus buffer of 6 characters. total = 6*10 + 9 + 6 = 75. 

Tables affected : rules , rules_history , rules_scheduled_changes, rules_scheduled_changes_history

Comment 10

[A.S.]

I'm unable to resolve these two issues : 
(1)  I was able to successfully upgrade the tables to newer version(023) , however I cannot downgrade it now. Here's the error log : https://pastebin.mozilla.org/8971637 .

(2) Modifying version column in Rules table in [db.py](https://github.com/mozilla/balrog/blob/fdfd220bf3cddc54c4a8123b016344ef0cb862f0/auslib/db.py#L1404) after successfully upgrading the tables and then running test cases resulted in failing TestDBModel.testColumnAttributesAreSameAsDb . Here's the error log : https://pastebin.mozilla.org/8971644 . This is strange as I've not modified rules.alias field at all ?

Comment 11

[[e-mail] (:bhearsum)]

(In reply to Ashish Sareen from comment #10)
> I'm unable to resolve these two issues : 
> (1)  I was able to successfully upgrade the tables to newer version(023) ,
> however I cannot downgrade it now. Here's the error log :
> https://pastebin.mozilla.org/8971637 .

This is most likely caused by a stale .pyc file. Try rm auslib/migrate/versions/*.pyc before you downgrade...

> (2) Modifying version column in Rules table in
> [db.py](https://github.com/mozilla/balrog/blob/
> fdfd220bf3cddc54c4a8123b016344ef0cb862f0/auslib/db.py#L1404) after
> successfully upgrading the tables and then running test cases resulted in
> failing TestDBModel.testColumnAttributesAreSameAsDb . Here's the error log :
> https://pastebin.mozilla.org/8971644 . This is strange as I've not modified
> rules.alias field at all ?

This is probably related to https://github.com/mozilla/balrog/pull/215. Let's talk about in the pull request.

Comment 12

[[e-mail] (:bhearsum)]

The first part of this is in production. We still need to follow-up with the widening of column - leaving open for that.

Comment 13

[[github robot]]

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

https://github.com/mozilla/balrog/commit/f01b12806862e58cc70547cd7be6a5fb61593c81
Bug 1257298 : Altering rules.version field's length from 10 to 75 (#233). r=bhearsum

Comment 14

[[e-mail] (:bhearsum)]

This in production and likely to be used very soon. Thank you!

Comment 15

[[e-mail] (:bhearsum)]

Rail just pointed out that we forgot to update the docs for this. Can you do that when you have time, Ashish?

Comment 16

[A.S.]

Updated the docs. https://github.com/mozilla/balrog/pull/271

Comment 17

[[github robot]]

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

https://github.com/mozilla/balrog/commit/3c6711fa6a7e72e641742ee610213a39533002d8
Bug 1257298 : updating docs (#271). r=bhearsum