Merge lp://staging/~rodsmith/hwcert-tools/get-ytd into lp://staging/~hardware-certification/hwcert-tools/reporting-tools

Hey.

Have a look at my comments below

Also, flake8 the script. I'm sure I've missed something. Also feel
free to pip install flake8-docstrings and re-flake the script.

On Wed, Mar 18, 2015 at 5:09 PM, Zygmunt Krynicki
<email address hidden> wrote:
> Review: Needs Fixing
>
> Hey.
>
> Have a look at my comments below
>
> Diff comments:
>
>> === added file 'certification_reports/get_ytd_makes.py'
>> --- certification_reports/get_ytd_makes.py 1970-01-01 00:00:00 +0000
>> +++ certification_reports/get_ytd_makes.py 2015-03-18 15:57:45 +0000
>> @@ -0,0 +1,131 @@
>> +#!/usr/bin/python2
>
> for all python2 code please do at a bare minimum:
>
> from __future__ import print_function, absolute_import
>
>> +'''
>> +Script to summarize make and model certification for the year to date
>
> Please move this to the description= keyword argument of the argparse.ArgumentParser() below
>
>> +or for a specified year. Produces a list of makes, the number of
>> +certificates issued for each make, and a list of the models certified
>> +for that make.
>> +
>> +Options:
>> +- C3 username
>> +- C3 API key (from https://certification.canonical.com/me/)
>> +- Ubuntu release (e.g., "precise" or "trusty")
>> +- Year (e.g., "2015")
>> +- --certnum={value} -- Certificate number for a report on one machine or
>> + "all" for all certified systems. Defaults to "all". Used for debugging,
>> + since retrieving all certificates takes ~10 minutes.
>> +
>> +Copyright (c) 2015 Canonical Ltd.
>
> please move the copyright to a comment at the top of the file
>
>> +
>> +Author: Rod Smith <email address hidden>
>> +'''
>> +
>> +from api_utils import APIQuery, QueryError
>
> Please put imports in this order: stdlib, 3rd party, this app/library. Put a newline between each group
>
>> +from argparse import ArgumentParser
>> +
>> +import sys
>> +import time
>> +
>> +C3_URL = "https://certification.canonical.com"
>> +CERTIFICATE_API = C3_URL + "/api/v1/certificates"
>> +
>> +SERVER_FF = ['Expansion Chassis',
>> + 'Main Server Chassis',
>> + 'Multi-system',
>> + 'Blade',
>> + 'Rack Mount Chassis',
>> + 'Server']
>> +
>> +api = APIQuery(C3_URL)
>> +
>> +def get_server_certs(username, api_key, release, year, certnum):
>> + '''
>> + Retrieve certificate information from C3
>
> Please follow PEP for docstrings
>
> Retrieve certificate information from C3.
>
> :param username:
> Bla bla bla
> :returns:
> Dictionary with ...
>
>> + Return value: Dictionary with manufacturer names as keys and
>> + list of models as values
>> + '''
>> + request_url = CERTIFICATE_API
>> + summaries = {}
>> +
>> + if certnum == "all":
>> + request_params = {"username": username,
>> + "api_key": api_key}
>> + else:
>> + request_params = {"username": username,
>> + "api_key": api_key,
>> + "name": certnum}
>> +
>> + certs = []
>> + try:
>> + certs = api.batch_query(request_url, params=request_params)
>> + except QueryError:
>> + print "Unable to get certificates"
>
> you can raise SystemExit("Unable to gett...."), this ...

On Wed, Mar 18, 2015 at 5:20 PM, Roderick Smith <email address hidden> wrote:
>
>
> Diff comments:
>
>> === added file 'certification_reports/get_ytd_makes.py'
>> --- certification_reports/get_ytd_makes.py 1970-01-01 00:00:00 +0000
>> +++ certification_reports/get_ytd_makes.py 2015-03-18 15:57:45 +0000
>> @@ -0,0 +1,131 @@
>> +#!/usr/bin/python2
>> +'''
>> +Script to summarize make and model certification for the year to date
>
> It's unclear to me how much of this you want moved. Also: MOVED or COPIED?

Moved, just keep a one-line summary at the top. The idea is to see
useful information when running get_ytd_makes.py --help.

Thanks
ZK

The latest changes to the output look good to me. I'll ack once Zyga is satisfied.

I'll do my best to help to resolve code style issues

On Wed, Mar 18, 2015 at 6:41 PM, Jeff Lane <email address hidden> wrote:
> The latest changes to the output look good to me. I'll ack once Zyga is satisfied.
> --
> https://code.launchpad.net/~rodsmith/hwcert-tools/get-ytd/+merge/253389
> You are reviewing the proposed merge of lp:~rodsmith/hwcert-tools/get-ytd into lp:~hardware-certification/hwcert-tools/reporting-tools.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/18/2015 12:09 PM, Zygmunt Krynicki wrote:
> Review: Needs Fixing
>
> Hey.
>
> Have a look at my comments below

I've changed most of these things. A few further comments and queries,
though....

>> +from api_utils import APIQuery, QueryError
>
> Please put imports in this order: stdlib, 3rd party, this
> app/library. Put a newline between each group

I think I've got this, but I'm really guessing at what's what. Is this
documented somewhere?

>> + num_of_make += 1 + grand_total += 1 +
>> print make.encode('utf-8') + ": " + str(num_of_make)
>
> This is a code smell, don't do this.
>
> Either move to all unicode strings internally or use all bytes and
> hope for the best. Why make needs this treatment now? I assume you
> get an unicode string from the API, correct?

The problem isn't with internal string handling; it's with what
happens when running the script with redirection when
".encode('utf-8')" is dropped:

$ ./get_ytd_makes.py {blah blah blah} > foo.txt
Traceback (most recent call last):
  File "./get_ytd_makes.py", line 137, in <module>
    sys.exit(main())
  File "./get_ytd_makes.py", line 119, in main
    print (make + ": " + str(num_of_make))
UnicodeEncodeError: 'ascii' codec can't encode characters in position
17-24: ordinal not in range(128)

It works fine when sending output to the console (or at least, an X
terminal), but when redirecting, it runs into this crash when a name
includes non-ASCII characters. (The trouble point for us is NEC, which
is encoded as "NEC Corporation (日本電気株式会社)"). I've Googled this
to death and using ".encode('utf-8')" is the only solution I've found
that works. (I've tried several ways of explicitly marking strings as
Unicode, and they all fail in one way or another.) If you have another
solution, I'm quite willing to try it.

- --
Rod Smith
Server and Cloud Certification Engineer
<email address hidden>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJVCdlhAAoJEFgyRI+V0FjmunMH/A+3z/c5e6L7WB3QVHaezDZx
jkvUeZjI3BPwKHI2jpdOiAxYQoGqK2RNMGL+3DZDVTiDYbbqOwG/13sZ6bWUXg2p
JT1Hvl8yUllOLjyys4HoiPskMjLYHS/4TUmOeJnWFMZ4yNY69Gzb08Y+emzavPDe
KZVw2Uxi36bZXBiO99U1SwmU0YcTx68oOt2hPlywOYsHtgQP1TKoGWb/iwrOUnVI
qE7W3Ho3zkc17LiQtNfPWUYvVlwFzd096LnhloSa6ujS3Yv7NUls6Va/X+vS/mcs
60dWgE0Agnc+6hCviOKNyEFcuKP/aJb/L5cd7uGjF7mUSBxEZeuyF7AVySMgROw=
=LSKD
-----END PGP SIGNATURE-----

Hey.

Thanks for improving this, it looks much better. I must ask you to run flake8 though as there are plenty of small things that flake will tell you about. Please look at inline comments for some extra details.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/18/2015 04:11 PM, Zygmunt Krynicki wrote:
> Review: Needs Fixing
>
> Hey.
>
> Thanks for improving this, it looks much better. I must ask you to
> run flake8 though as there are plenty of small things that flake
> will tell you
about.

I'd done that before, but I hadn't had the flake8-docstrings
installed. I've now installed it and it's coming up clean.

A couple more comments below....

>> + if certnum == "all": + request_params = {"username":
>> username, + "api_key": api_key} +
>> else: + request_params = {"username": username, +
>> "api_key": api_key, + "name": certnum}
>
> Don't add empty lines within functions.

Sorry, I must disagree on this one. From a cognitive point of view,
blank lines help separate logically-related sections of functions,
which helps both finding relevant sections of code and understanding
it. In Googling the issue, I found, in PEP-8:

: Use blank lines in functions, sparingly, to indicate logical
: sections.

Judging by comments on online forums, this sentence seems to be
interpreted as anything from "use as many blank lines as you want" to
"don't ever use blank lines." In the face of that ambiguity, I'm
sticking with what both my personal preference and my cognitive
psychology training says works.

>> + "https://certification.canonical.com/me/") +
>> parser.add_argument( + "release", help="Ubuntu release
>> ('trusty', 'precise', etc.)")
>
> don't enumerate this but add choices=('trusty', ...) this will let
> argparse check it automatically

The downside to this is that it will require changes for each new
version. Granted, that's not often, but I'd rather not deal with that
(or, worse, foist it onto somebody who's never seen the code) when
16.04 or 18.04 comes out. As it is now, the script returns a report
with no entries if the code name is mistyped, which IMHO is not a big
deal; and it will work with future releases. Thus, I'd prefer to leave
it as-is.

- --
Rod Smith
Server and Cloud Certification Engineer
<email address hidden>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJVCel3AAoJEFgyRI+V0FjmH4YH/1z1sQGyB5bcE+m2JLTPHt4Y
hZXxJAD81UgRSarVmKwNPgEbtJ+zrk5isMnE+A/e4ZpTisPcZLNjwNA9vyT5jY+k
3AhTv4bZiDQGhXcFrbLZ8mJXYzcbeGo2aboPRljrBs+2SBKuuS6OM02MaKdYQaBs
423F+0pDgZkHHue3DmP/2cMvgTLLRue25QKiP3sncj1ur695Efy2NpSgeZo1j/pd
i5MfbbPY240WsbLgigKJJRWLxBfVhztXPlwPSqnESMMbEyxxNybPL59fYz9KC9+o
pX+4dPbVBTCD7GVO2f4hipfnJWzZqOtH9fltyv5m4A5JJCy4OasxeWwF0xl6Xwg=
=2kQB
-----END PGP SIGNATURE-----

On Wed, Mar 18, 2015 at 10:09 PM, Roderick Smith
<email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 03/18/2015 04:11 PM, Zygmunt Krynicki wrote:
>> Review: Needs Fixing
>>
>> Hey.
>>
>> Thanks for improving this, it looks much better. I must ask you to
>> run flake8 though as there are plenty of small things that flake
>> will tell you
> about.
>
> I'd done that before, but I hadn't had the flake8-docstrings
> installed. I've now installed it and it's coming up clean.
>
> A couple more comments below....
>
>>> + if certnum == "all": + request_params = {"username":
>>> username, + "api_key": api_key} +
>>> else: + request_params = {"username": username, +
>>> "api_key": api_key, + "name": certnum}
>>
>> Don't add empty lines within functions.
>
> Sorry, I must disagree on this one. From a cognitive point of view,
> blank lines help separate logically-related sections of functions,
> which helps both finding relevant sections of code and understanding
> it. In Googling the issue, I found, in PEP-8:
>
> : Use blank lines in functions, sparingly, to indicate logical
> : sections.
>
> Judging by comments on online forums, this sentence seems to be
> interpreted as anything from "use as many blank lines as you want" to
> "don't ever use blank lines." In the face of that ambiguity, I'm
> sticking with what both my personal preference and my cognitive
> psychology training says works.

Don't ever use two newlines. That's clearly wrong. I don't think this
trivial function warrants to have any newlines but if you want to keep
the single ones by all means do that.
>
>>> + "https://certification.canonical.com/me/") +
>>> parser.add_argument( + "release", help="Ubuntu release
>>> ('trusty', 'precise', etc.)")
>>
>> don't enumerate this but add choices=('trusty', ...) this will let
>> argparse check it automatically
>
> The downside to this is that it will require changes for each new
> version. Granted, that's not often, but I'd rather not deal with that
> (or, worse, foist it onto somebody who's never seen the code) when
> 16.04 or 18.04 comes out. As it is now, the script returns a report
> with no entries if the code name is mistyped, which IMHO is not a big
> deal; and it will work with future releases. Thus, I'd prefer to leave
> it as-is.

There's a python module with all known ubuntu releases but I forgot
how it's called. If you rather not do validation and rely on later
failures then that's okay too.

Thanks
ZK

The only last comment is on handling the .encode bit. That's still totally broken. There are well-known methods for handling bytes and unicode in python2. The general rule is to *always* encode at boundaries (on the outside world is bytes, on the inside text is unicode, bytes are str).

You can try this:
 - assume all the api query functions return unicode strings
 - add __from__ future import unicode_literals
 - you will get everything as unicode internally
 - each file IO needs to have the mode specified as either text with encoding= keyword argument or as 'b' and never treat that as text.

This will give you a clear upgrade path to python3 in any situation. It also means that you never have to encode anything.

There are (and were) no double newlines within either of the script's functions.

As to the .encode('utf-8'), I've found a way to do it without that. FWIW, the issue is that Python doesn't know the proper encoding when redirecting, so it has to be told that explicitly, as described here:

http://stackoverflow.com/questions/4545661/unicodedecodeerror-when-redirecting-to-file