canonical-ubuntu-qa team mailing list archive

[Dave Jones <mp+460847@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Left a few diff comments -- I very much like this approach though!

Diff comments:

> diff --git a/charms/focal/autopkgtest-web/webcontrol/sqlite-writer b/charms/focal/autopkgtest-web/webcontrol/sqlite-writer
> new file mode 100755
> index 0000000..3939bde
> --- /dev/null
> +++ b/charms/focal/autopkgtest-web/webcontrol/sqlite-writer
> @@ -0,0 +1,136 @@
> +#!/usr/bin/python3
> +
> +import configparser
> +import json
> +import logging
> +import os
> +import socket
> +import sqlite3
> +import urllib.parse
> +
> +import amqplib.client_0_8 as amqp
> +from helpers.utils import init_db
> +
> +EXCHANGE_NAME = "sqlite-write-me.fanout"
> +
> +config = None
> +db_con = None
> +
> +INSERT_INTO_KEYS = [
> +    "test_id",
> +    "run_id",
> +    "version",
> +    "triggers",
> +    "duration",
> +    "exitcode",
> +    "requester",
> +    "env",
> +    "uuid",
> +]
> +
> +
> +def amqp_connect():
> +    """Connect to AMQP server"""
> +
> +    cp = configparser.ConfigParser()
> +    cp.read(os.path.expanduser("~ubuntu/autopkgtest-cloud.conf"))
> +    amqp_uri = cp["amqp"]["uri"]
> +    parts = urllib.parse.urlsplit(amqp_uri, allow_fragments=False)
> +    amqp_con = amqp.Connection(
> +        parts.hostname, userid=parts.username, password=parts.password
> +    )
> +    logging.info(
> +        "Connected to AMQP server at %s@%s" % (parts.username, parts.hostname)
> +    )
> +
> +    return amqp_con
> +
> +
> +def db_connect():
> +    """Connect to SQLite DB"""
> +    cp = configparser.ConfigParser()
> +    cp.read(os.path.expanduser("~ubuntu/autopkgtest-cloud.conf"))

My same old whinge about ConfigParser.read :)

> +
> +    db_con = init_db(cp["web"]["database"])
> +
> +    return db_con
> +
> +
> +def check_msg(queue_msg):
> +    required_keys = [
> +        "test_id",
> +        "run_id",
> +        "version",
> +        "triggers",
> +        "duration",
> +        "exitcode",
> +        "requester",
> +        "env",
> +        "uuid",
> +    ]
> +    queue_msg_keys = list(queue_msg.keys())
> +    required_keys.sort()
> +    queue_msg_keys.sort()
> +    if queue_msg_keys == required_keys:

No need to go sorting lists; just compare sets. Replace [] with {} in required_keys, and convert queue_msg.keys to a set() instead of a list().

If queue_msg is a dict (or the result of the queue_msg.keys() method conforms to KeysView), you don't even need to convert it to a set(), you can just compare queue_msg.keys() == required_keys.

> +        return True
> +    return False
> +
> +
> +def process_message(msg, db_con):
> +    # aight, time to test this with download-results and download-all-results now.
> +    body = msg.body
> +    if isinstance(body, bytes):
> +        body = body.decide("UTF-8", errors="replace")

I think you mean decode? :)

> +    info = json.loads(body)
> +    logging.info("Message is: \n%s" % json.dumps(info, indent=2))
> +    print(check_msg(info))
> +    if not check_msg(info):
> +        logging.info(
> +            "Message has incorrect keys!\n%s" % json.dumps(info, indent=2)
> +        )
> +    # insert into db
> +    try:
> +        with db_con:
> +            c = db_con.cursor()
> +            c.execute(
> +                "INSERT INTO result VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)",

Nitpick: I don't like bare INSERTs where the columns are implicit; it's a bit like using SELECT * -- I never do it in production code because it makes the code resistant to (benign) schema changes. If the ordering of the columns of the table changes, or new columns with valid defaults are introduced (e.g. an "inserted on timestamp" column which defaults to "now"), this INSERT will fail when it should calmly carry on doing the Right Thing.

In other words, this should be INSERT INTO result(col1, col2, ...) VALUES (?, ?, ...)

Really really nitpicky: Whilst the ? paramstyle is the SQL standard, it's also rubbish. The sqlite3 module is thankfully one of those that sensibly varied from the Python DB-API and permitted this to be modified. You may want to change sqlite3.paramstyle to "named" (the other supported value, and the One True Paramstyle ;) and use :params instead.

> +                (
> +                    info["test_id"],
> +                    info["run_id"],
> +                    info["version"],
> +                    info["triggers"],
> +                    info["duration"],
> +                    info["exitcode"],
> +                    info["requester"],
> +                    info["env"],
> +                    info["uuid"],
> +                ),
> +            )
> +            db_con.commit()

This is redundant; you're already in a "with db_con" block which will implicitly commit / rollback as appropriate

> +        logging.info("Inserted the following entry into the db:\n%s" % body)
> +    except sqlite3.OperationalError as e:
> +        logging.info("Insert operation failed with: %s" % str(e))
> +    except sqlite3.IntegrityError as e:
> +        logging.info("Insert operation failed with: %s" % str(e))

Do we really just want to log an info message if this stuff fails? At the very least I'd expect to log an error-level message, but I'd also want to raise the exception and crash in most instances? Or do we expect to go inserting duplicate keys?

Also, may be worth looking at logging.exception for these (which implicitly records exception information from within the handler)

> +
> +    msg.channel.basic_ack(msg.delivery_tag)
> +
> +
> +if __name__ == "__main__":
> +    logging.basicConfig(level=logging.INFO)
> +    db_con = db_connect()
> +    amqp_con = amqp_connect()
> +    status_ch = amqp_con.channel()
> +    status_ch.access_request("/complete", active=True, read=True, write=False)
> +    status_ch.exchange_declare(
> +        EXCHANGE_NAME, "fanout", durable=True, auto_delete=False
> +    )
> +    queue_name = "sqlite-writer-listener-%s" % socket.getfqdn()
> +    status_ch.queue_declare(queue_name, durable=True, auto_delete=False)
> +    status_ch.queue_bind(queue_name, EXCHANGE_NAME, queue_name)
> +    logging.info("Listening to requests on %s" % queue_name)
> +    status_ch.basic_consume(
> +        "", callback=lambda msg: process_message(msg, db_con)
> +    )
> +    while status_ch.callbacks:
> +        status_ch.wait()


-- 
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/460847
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~andersson123/autopkgtest-cloud:d-r-d-a-r-merging into autopkgtest-cloud:master.