Detect and report changes about Smart package locks

Nice branch! Just one comment:

[1] The sequence_to_ranges calls are not tested (although I'm not sure how useful they are).

+1!

Thanks, Thomas!

[1]

I've added two more tests for them. Having sequences of consecutive ids is probably rare in this case, but I guess it doesn't hurt to have it.

[1]

+ @param relation, version: The condition the package version must
+ satisfy in order to be locked.

The Epytext syntax used here is incorrect, I believe.

[2]

+ def set_package_lock(self, name, relation="", version=""):

and then:

+ # Smart wants None here
+ if relation == "":
+ relation = None
+ if version == "":
+ version = None

I think you should use None as the default values. It will allow
you to remove the conversion code you have and shouldn't have any
impact on the behaviour of _validate_lock_condition.

[3]

+ def remove_package_lock(self, name, relation="", version=""):

and then:

+ # Smart wants () here
+ if not relation:
+ relation = ()
+ if not version:
+ version = ()

I recommend you use () as the default values for relation and
version here. It will allow you to remove the conversion code
without impacting the other surrounding code.

[4]

+ if set_package_locks:
+ message["set"] = sorted(set_package_locks)
+ if unset_package_locks:
+ message["unset"] = sorted(unset_package_locks)

Would you mind calling these keys 'lock' and 'unlock'. I realize
that's pretty nit-picky, but it feels a bit more clear to me.

[5]

+ If Smart indicates us locked packages we didn't know about, report

s/us// please.

[6]

+ Locked packages whose hashes are unkwnown don't get reported.

s/unkwnown/unknown/ please.

[7]

+ cursor.execute("CREATE TABLE package_locks"
+ " (name TEXT NOT NULL, relation TEXT, version TEXT,"
+ " UNIQUE(name, relation, version))")
+ cursor.execute("CREATE TABLE locked"
+ " (id INTEGER PRIMARY KEY)")

You need to add a patch to correctly upgrade existing package store
databases.

Thanks for reviewing, Jamu!

[1], [5], [6]

Fixed

[2], [3]

Yeah, I had noticed that as well, but I basically wanted to keep the default values of these "set" methods consistent with the values returned by the "get" ones. I agree it introduces unnecessary complexity though, so I changed the code as you suggest.

[4]

What about "added" and "removed" instead? The fact is that "lock" and "unlock" are actually further consequences the package locks themselves have on the packages, at this point we're really just reporting about newly "added" or "removed" package locks.

Another even better option for me would be "set" and "removed", which matches exactly the smart command line terminology, where you type things like:

smart flag --set lock "foo > 1.0"

and

smart flag --remove lock "foo > 1.0"

[7]

Absolutely a patch system would be better.

However we don't have any in place yet, and the code as it is now would actually succeed in upgrading an existent database, because the two CREATE TABLE statements are performed first, and they will succeed as the two tables don't exist yet. I guess a proper patch system would probably need a separate branch, I would personally postpone it till when it's actually necessary, I don't feel strong on this though.

Free:

[4]

I'd be happy with "create" and "delete", and in fact, that would
match the hardware inventory plugin. Anyway, please do what you
feel is best, it was really just a minor suggestion.

[7]

I guess running queries that fail doesn't raise OperationalError and
so the transaction ends up getting committed...? Have you verified
that this code works with existing installations?

Jamu:

[4]

I went for "created" and "deleted", using the past participle because it refers to something that is indeed already happened (like the "installed" field of the packages message typ
[7]

Yes, I tested this in real-world and it works. The OperationalError exception is raised, and the transaction is rolled back, however the rollback preserves tables created during the transaction, weirdly enough it must be a Sqlite "feature" :) I've added an explicit unit test for this, so we can catch possible changes of this behavior across different Ubuntu releases (thanks buildbot!)

Got a verbal +1 from Jamu during a Skype call.

Confirmed in staging.

This was released already for the mentioned distros bar Intrepid, which is EOL and I didn't check.