[pkg-apparmor] UDD notifications script

[u]

Hi,

thank you both Christian and intrigeri for the valuable input.
I'm having a lot of fun implementing the corrections :)

intrigeri:
> u wrote (19 Feb 2015 17:20:52 GMT) :
>> I think I've applied everything you mentioned and share your vision.
> Yay, congrats, I find it a lot more readable and cleaner. Thanks! :)

> * This looks suboptimal (but of course, as long as we haven't
>   a massive amount of usertagged bugs, it should be just fine):
> 
>  	for bug in cursor.fetchall():
> 		buglist.append(bug)
> 
>   ... so I'm curious: can't we do that in one batch operation instead?

Done, using Christian's suggestion.

> * When using the get_bug_list output (a list of pairs) in
>   send_team_notification, we have to access it by indices: bug[0] to
>   get the bug ID, bug[1] to get the added or removed usertag. It's not
>   astonishingly obvious. I would recommend that get_bug_list outputs:
> 
>   - either a list of dictionaries so we can then write bug['id']
>     and bug['usertag']
>   - or a dictionary whose keys would be the bug IDs, and values would
>     be the usertag; but wait! this would be too limiting, read on:

> * I see we first retrieve a list of usertagged bugs (in get_bug_list),
>   and then run one more query per bug that had a usertag added or
>   removed, in order to get its title. Alternatively, get_bug_list
>   could use a JOIN to retrieve all the data we need about all
>   usertagged bugs in one go. No idea the higher number of queries
>   (current implementation) costs more than retrieving the title of
>   bugs that we won't use (suggested implementation). Anyhow, putting
>   aside performance and resources considerations, I would find the
>   implementation of send_team_notification more readable and elegant
>   if get_bug_list returned a list of dictionaries, each such
>   dictionary having keys (id, title, usertag). Food for thought :)

I finally did that by joining the two tables and it the query still
seems to be done quite quickly.

> * pylint says that "def get_bug_title(bugid) :" has one too much
>   space. It's also quite unhappy about a lot of other small things.
>   Possibly it knows Python best practices better than you and I.
>   I've no idea (really -- it might produce obsolete recommendations,
>   and I've not checked). Similarly, pyflakes is a bit mean and detects
>   two unused variables.

I will run this on the new script too.

> * In:
> 
> 	cursor.execute("SELECT title from bugs WHERE id='%s'" % bugid)
> 	for bug in cursor.fetchall():
> 		return bug[0]
> 
>   ... we're somewhat acting defensively as if we didn't have
>   a guarantee that the id is a unique identifier (is it the case?),
>   which can arguably be a good thing.
> 
>   But we're actually not defensive in practice, since we're always
>   returning the title of the first matching bug and ignoring any
>   other. So, retrieving all the data using fetchall(), and then
>   iterating on the first result only in a for loop feels... weird.
> 
>   So I would simply use fetchone() instead of fetchall(), and drop the
>   for loop entirely.

Tried that in a previous iteration and i worked. But I dropped it since
as I don't need it anymore.

>   Now, if you really want to be more defensive, we can also count the
>   number of matching bugs and throw an exception if it's 2 or more,
>   but I suspect we could avoid that after a quick look at the
>   UDD schema. What's an ID if it's not an ID? :)
> 
> * Mixing tabs and spaces for indentation produces very confusing
>   results in my $EDITOR:
> 
> 	except IOError as e:
> 		send_error_mail("Could not save state file.")
>         return False
> 
> 	return True
> 
>   ... here, the "return False" statement is really part of the
>   exception handling block, even if it doesn't look like it when one
>   has tabs display width = 8 spaces. I suggest applying the
>   indentation changes suggested by pylint to avoid that :)

OK. Corrected.

> * In the read_statefile function, I believe that the "return old"
>   statement has no good reason to be in the try block. In general it's
>   good practice to guard as little code as possible with exception
>   catching blocks. In this case, as a non-expert in Python, I have no
>   idea if it may have any adversarial consequences in practice to
>   leave it there, but IMO it would be good code hygiene to move it
>   after the exception handling block. And by the way, it'll make it
>   clearer what is the expected output of the function on success :)

Ok. Corrected.

> * 'print "No new data."' might produce useless noise when run from
>   cron, no?

Deleted.

> * The 'else' on line 93 (at commit 40226e0) forces us to indent the
>   main algorithm of this function. I generally prefer dropping such
>   else clauses, and instead return earlier in case we cannot do our
>   job... which is actually already done in this function, so the
>   refactoring I'm suggesting is trivial.

Indeed.

> * The send_team_notification function still uses the sender and
>   receiver global variables. Any reason why?

I modified this. I kept them global in first place because the other
option (which I am using now) needs to pass about 5 parameters to that
function.

> * It seems that we're passing to compare_state:
>    - a string we got from read_statefile
>    - a list we got from get_bug_list
>   ... and then compare_state treats both as strings. For some reason,
>   it works anyway.
>
>   I think I'd find the design cleaner if compare_state took structured
>   data as input (a set, a dictionary, whatever it prefers). And then,
>   read_statefile should handle deserialization and return data in that
>   same format. And then, we would be handling {,}deserialization only
>   in save_statefile and read_statefile, and using only structured data
>   anywhere else, which feels better to me.

Indeed.

I was not very firm with saving the data to a file and thus ended up
with a weird converstion from a saved string to a set in first place.
But now I've seen that the pickle module can save and read dictionaries
just fine and changed the way I save the data. Thus, I can now use those
instead.

> * compare_state has a comment that says "convert new state string to
>   dictionary", while it's apparently (from my Python almost-newbie
>   PoV) creating a Set object instead. Who's right? :)

Your newbie PoV is right :)
As we use the dictionary list now, this has become obsolete.

> * The "Send the message via our local SMTP server" comment feels
>   wrong, now that you've made the SMTP relay configurable.

Corrected.

u.