canonical-hw-cert team mailing list archive

[Massimiliano Girardi <mp+447532@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

A few tweeks proposals but overall very good job!

I seem to have a few changes when running black on the file, maybe run it before submitting the update

Diff comments:

> diff --git a/add_private_ppa.py b/add_private_ppa.py
> new file mode 100644
> index 0000000..d8228bd
> --- /dev/null
> +++ b/add_private_ppa.py
> @@ -0,0 +1,240 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright 2023 Canonical Ltd.
> +# Written by:
> +#   Maciej Kisielewski <maciej.kisielewski@xxxxxxxxxxxxx>
> +
> +"""
> +This program adds a private PPA to the system.
> +
> +It does so by creating a credentials file that holds the login and password,
> +so they are not stored in plaintext in the URIs in the sources.list file.
> +
> +Then the URL (without the login and password) is added to the sources.list file.
> +
> +Finally, the PPA's key is added to the system.
> +"""
> +
> +import argparse
> +import logging
> +import os
> +import subprocess
> +import textwrap
> +from urllib.parse import urlparse
> +
> +logging.basicConfig(level=logging.INFO)
> +logging.getLogger().name = os.path.basename(__file__)

I don't like this, it changes the global (logging) scope for no reason. Why don't `logger = logging.getLogger(name=...)` and then use the local logger of this module?

> +
> +
> +def guess_ubuntu_codename() -> str:
> +    """
> +    Guess the Ubuntu codename.
> +
> +    The codename is guessed by running the lsb_release command.
> +    """
> +    try:
> +        logging.info("Guessing Ubuntu codename...")
> +        codename = subprocess.check_output(
> +            ["lsb_release", "--codename", "--short"], universal_newlines=True
> +        ).strip()
> +        logging.info("Ubuntu codename guessed: %s", codename)
> +        return codename
> +    except FileNotFoundError:
> +        logging.error("lsb_release not found!")
> +        raise SystemExit(1)

this is not properly an error, this is critical, I would either make it critical or `raise SystemError("lsb_release not found!")`. It would not respect the logger formatting but it is pretty neat and saves a line

> +    except (subprocess.CalledProcessError, OSError) as e:
> +        logging.error("Problem encountered when running lsb_release: %s", e)
> +        raise SystemExit(1)

same here

> +
> +
> +def trust_the_key(key: str) -> None:
> +    """
> +    Trust the key.
> +
> +    The key is trusted by running the apt-key command.
> +    """
> +    try:

This function uses the same "pattern" as above more or less, I would wrap them in a single error handling function. The real function on both here is what is contained in `try:` according to me, everything else is the same

> +        logging.info("Trusting the key...")
> +        subprocess.check_call(["apt-key", "add", key])
> +        logging.info("Key trusted.")
> +    except FileNotFoundError:
> +        logging.error("apt-key not found!")
> +        raise SystemExit(1)
> +    except subprocess.CalledProcessError as e:
> +        logging.error("Problem encountered when running apt-key: %s", e)
> +        raise SystemExit(1)
> +
> +
> +def slugify_name(name: str) -> str:
> +    """
> +    Slugify a name, so it can be used as a filename.
> +    The characters that are not allowed in filenames are replaced with a dash.
> +    The characters being replaced are: '/', '\', ':', '*', '?', '"', '<', '>', '|', and space.
> +
> +    >>> slugify_name("")
> +    ''
> +    >>> slugify_name("a")
> +    'a'
> +    >>> slugify_name("a/b")
> +    'a-b'
> +    >>> slugify_name("a\\\\b") # double escaping because of doctest
> +    'a-b'
> +    >>> slugify_name("a:b")
> +    'a-b'
> +    >>> slugify_name("a*b")
> +    'a-b'
> +    >>> slugify_name("a?b")
> +    'a-b'
> +    >>> slugify_name('a"b')
> +    'a-b'
> +    >>> slugify_name("a<b")
> +    'a-b'
> +    >>> slugify_name("a>b")
> +    'a-b'
> +    >>> slugify_name("a|b")
> +    'a-b'
> +    >>> slugify_name("a b")
> +    'a-b'
> +    >>> slugify_name(r"a/b\\:*?\\"<>| ")
> +    'a-b----------'
> +    """
> +    return (

I would just `re.sub(r"[^\w]", "-")`, it is not exactly the same thing but how much more variations do we want a file name to be able to have?

> +        name.replace("/", "-")
> +        .replace("\\", "-")
> +        .replace(":", "-")
> +        .replace("*", "-")
> +        .replace("?", "-")
> +        .replace('"', "-")
> +        .replace("<", "-")
> +        .replace(">", "-")
> +        .replace("|", "-")
> +        .replace(" ", "-")
> +    )
> +
> +
> +def create_apt_auth_file(ppa: str, login: str, password: str) -> None:
> +    """
> +    Create a credentials file for the PPA.
> +
> +    The file will be named after the PPA's name, with the login and password
> +    appended to it, and will be placed in the /etc/apt/auth.conf.d/ directory.
> +    """
> +    ppa_name = slugify_name(extract_ppa_name(ppa))
> +    auth_file_path = "/etc/apt/auth.conf.d/{}.conf".format(ppa_name)

Minor: this is done twice, this line can be removed (although I feel like this variable name is better than `auth_file`)

> +    contents = textwrap.dedent(
> +        """
> +        machine {}
> +        login {}
> +        password {}
> +        """
> +    ).format(ppa, login, password)
> +
> +    auth_file = "/etc/apt/auth.conf.d/ppa-{}.conf".format(ppa_name)
> +    if os.path.exists(auth_file):
> +        logging.warning("Credentials file already exists: %s", auth_file)
> +        logging.warning("Not overwriting it.")
> +    else:
> +        with open(auth_file, "w") as f:
> +            f.write(contents)
> +        logging.info("Created credentials file: %s", auth_file)
> +
> +
> +def extract_ppa_name(url: str) -> str:
> +    """
> +    Extracts the PPA address from a URL.
> +
> +    >>> extract_ppa_name('https://private-ppa.launchpadcontent.net/test/test-path')
> +    'test/test-path'
> +
> +    >>> extract_ppa_name('https://private-ppa.launchpadcontent.net/singlepath')
> +    'singlepath'
> +
> +    >>> extract_ppa_name('https://private-ppa.launchpadcontent.net/')
> +    ''
> +
> +    >>> extract_ppa_name('https://private-ppa.launchpadcontent.net')

This should probably raise an exception

> +    ''
> +
> +    >>> extract_ppa_name('not-a-url')
> +    'not-a-url'

also this one, the param is called url, if I provide not-a-url I should get an error back

> +    """
> +    parsed_url = urlparse(url)
> +    path = parsed_url.path
> +    # Remove initial slash
> +    if path.startswith("/"):
> +        path = path[1:]
> +    return path
> +
> +
> +def add_ppa_to_sources_list(ppa: str) -> None:
> +    """
> +    Add the PPA to the sources.list file.
> +
> +    The PPA's URL will be added to the sources.list file, with the login and
> +    password replaced with the name of the credentials file.
> +    """
> +    ppa_name = slugify_name(extract_ppa_name(ppa))
> +    sources_list_file = "/etc/apt/sources.list.d/{}.list".format(ppa_name)
> +    release_codename = guess_ubuntu_codename()
> +    contents = textwrap.dedent(
> +        """
> +        deb {ppa} {release_codename} main
> +        deb-src {ppa} {release_codename} main
> +        """
> +    ).format(ppa=ppa, release_codename=release_codename)
> +
> +    if os.path.exists(sources_list_file):
> +        logging.warning(
> +            "Sources list file already exists: %s", sources_list_file
> +        )
> +        logging.warning("Not overwriting it.")
> +    else:
> +        with open(sources_list_file, "w") as f:
> +            f.write(contents)
> +        logging.info("Created sources list file: %s", sources_list_file)
> +
> +
> +def add_ppa_key(key: str) -> None:
> +    """
> +    Add the PPA's key to the system.
> +
> +    The PPA's key will be added to the system by running the apt-key command.
> +    """
> +    try:
> +        cmd = [
> +            "apt-key",
> +            "adv",
> +            "--keyserver",
> +            "keyserver.ubuntu.com",
> +            "--recv-keys",
> +            key,
> +        ]
> +
> +        logging.info("Adding the PPA's key using apt-key...")
> +        subprocess.check_call(cmd)
> +        logging.info("PPA's key added.")
> +    except FileNotFoundError:
> +        logging.error("apt-key not found!")
> +        raise SystemExit(1)
> +    except subprocess.CalledProcessError as e:

same as above

> +        logging.error("Problem encountered when running apt-key: %s", e)
> +        raise SystemExit(1)
> +
> +
> +def main() -> None:
> +    print(guess_ubuntu_codename())

dd

> +    parser = argparse.ArgumentParser(
> +        description="Add a private PPA to the system."
> +    )
> +    parser.add_argument("ppa", help="The URL of the PPA to add.")
> +    parser.add_argument("login", help="The login to use for the PPA.")
> +    parser.add_argument("password", help="The password to use for the PPA.")
> +    parser.add_argument("key", help="PPA's key to add.")
> +    args = parser.parse_args()
> +    create_apt_auth_file(args.ppa, args.login, args.password)
> +    add_ppa_to_sources_list(args.ppa)
> +    add_ppa_key(args.key)

I would swap this two around, just so that if the script fails at 241 (you close it for instance) you are not stuck with a ppa for whom you don't have the key trusted

> +
> +
> +if __name__ == "__main__":
> +    main()


-- 
https://code.launchpad.net/~kissiel/hwcert-jenkins-tools/+git/hwcert-jenkins-tools/+merge/447532
Your team Canonical Hardware Certification is subscribed to branch hwcert-jenkins-tools:master.