Merge into reporting-tools : get-ytd : Code : Hardware Certification Tools

Status:	Superseded
Proposed branch:	lp://staging/~rodsmith/hwcert-tools/get-ytd
Merge into:	lp://staging/~hardware-certification/hwcert-tools/reporting-tools
Diff against target:	141 lines (+137/-0) 1 file modified certification_reports/get_ytd_makes.py (+137/-0)
To merge this branch:	bzr merge lp://staging/~rodsmith/hwcert-tools/get-ytd
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Zygmunt Krynicki (community)		Needs Fixing on 2015-03-18
Jeff Lane 	2015-03-18	Pending
Review via email: mp+253389@code.staging.launchpad.net

Revision history for this message

Zygmunt Krynicki (zyga) wrote on 2015-03-18:

#

Hey.

Have a look at my comments below

review: Needs Fixing

Revision history for this message

Zygmunt Krynicki (zyga) wrote on 2015-03-18:

#

Download full text (7.4 KiB)

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 ...

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
<zygmunt.krynicki@canonical.com> 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 <rod.smith@canonical.com>
>> +'''
>> +
>> +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 will behave better than printing this manually. You also don't need the sys.exit() call anymore
>
>> +        sys.exit(1)
>> +    certs = [cert for cert in certs if cert['name']]
>> +
>> +    for cert in certs:
>> +        if cert['created_at'][0:4] == year:
>> +            machine = api.single_query(C3_URL + cert['machine'],
>> +                                       params=request_params)
>> +            if (machine['platform']['form_factor'] in SERVER_FF and
>> +                    cert['release']['codename'] == release):
>> +                make = machine['account']['name']
>> +                details = {'model': machine['platform']['name'],
>> +                           'certnum': cert['name'],
>> +                           'cid': machine['canonical_id'].encode('utf-8')}
>> +                try:
>> +                    summaries[make].append(details)
>> +                except:
>
> Please never use blank except lists. What are you expecting to catch here?
>
>> +                    summaries[make] = [details]
>> +
>> +    return summaries
>> +
>> +
>> +def main():
>> +    parser = ArgumentParser(description="A YTD report on certificates " +
>
> It's my personal preference but I'd rather format this whole section like this:
>
> parser = argparse.ArgumentParser(
>    ...)
> parser.add_argument(
>    '...', foo='...',
>    bar='...')
> ...
>
> This is more consistent as typically all of this is wrong and wraps poorly
>
>> +                            "issued, by make.")
>> +    parser.add_argument('username',
>> +                        help="Launchpad username used to access C3.")
>> +    parser.add_argument('api_key',
>> +                        help="API key used to access C3.")
>> +    parser.add_argument("release", help="Ubuntu release ('trusty', " +
>> +                        "'precise', etc.)")
>> +    parser.add_argument("year", help="The year to summarize")
>> +    parser.add_argument("--certnum", help="Certificate number or 'all'" +
>> +                        "(used for debugging)", default="all")
>> +    args = parser.parse_args()
>> +
>> +    summaries = get_server_certs(args.username, args.api_key, args.release,
>> +                                 args.year, args.certnum)
>> +
>> +    print "YTD server certificates for " + args.release + " in " + args.year
>
> please use str.format() for that
>
>> +    print "\nReport generated: %s\n"  % time.strftime("%c")
>> +
>> +    # First pass: Generate "executive summary"
>> +    print "Summary:"
>> +    print "--------\n"
>> +    grand_total = 0
>> +    for make in sorted(summaries):
>> +        num_of_make = 0
>> +        for model in sorted(summaries[make]):
>
> If distinct values of "make" repeat at all I'd rather keep a cached copy of sorted(summaries[make]) to lower complexity. You only need to calculate that if make changes from last_make
>
>> +            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?
>
>> +    print "\nGrand total: ", grand_total
>> +
>> +    # Second pass: Generate detailed report
>> +    print "\n\nDetailed information:"
>> +    print "---------------------\n"
>> +    for make in sorted(summaries):
>> +        print make.encode('utf-8') + ":"
>
> Same comment as above
>
>> +        num_of_make = 0
>> +        for model in sorted(summaries[make]):
>
> Same comment as in the other loop.
>
>> +            num_of_make += 1
>> +            print (" * " + model['model'] + " (CID " + model['cid'] +
>
> Please use str.format  instead
>
>> +                   "):")
>> +            print ("    https://certification.canonical.com/certificates/" +
>> +                   model['certnum'])
>> +        print make.encode('utf-8'), " TOTAL: ", num_of_make, "\n"
>
> Same comment, explicit .encode is a code smell
>
>> +
>> +
>> +if __name__ == "__main__":
>> +    sys.exit(main())
>>
>
>
> --
> 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.

Revision history for this message

Rod Smith (rodsmith) on 2015-03-18:

#

Revision history for this message

Zygmunt Krynicki (zyga) wrote on 2015-03-18:

#

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

Revision history for this message

Jeff Lane  (bladernr) wrote on 2015-03-18:

#

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

Revision history for this message

Zygmunt Krynicki (zyga) wrote on 2015-03-18:

#

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.

lp://staging/~rodsmith/hwcert-tools/get-ytd updated on 2015-03-18

170. By Rod Smith on 2015-03-18: Modifications to get_ytd_makes.py in response to Zygmunt's comments

Revision history for this message

Rod Smith (rodsmith) wrote on 2015-03-18:

#

-----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-----

-----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
rod.smith@canonical.com
-----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-----

lp://staging/~rodsmith/hwcert-tools/get-ytd updated on 2015-03-19

171. By Rod Smith on 2015-03-18: More tweaks to get_ytd_makes.py in response to Zygmunt's comments
172. By Rod Smith on 2015-03-18: More changes in response to Zygmunt's comments
173. By Rod Smith on 2015-03-19: Changes to get_ytd_makes.py's Unicode handling in response to Zygmunt's comments

Hardware Certification Tools

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers