canonical-hw-cert team mailing list archive

[Maciej Kisielewski <mp+447532@xxxxxxxxxxxxxxxxxx>]

Either we have different blacks installed, or you have yours configured to use a long line length.

I've updated the branch to satisfy almost all of your comments. Squashed and pushed as the policy dictates for LP MRs.


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

The default logger is always there. I really couldn't see what making another instance of a logger would solve.
Also the setup of the local logger to achieve the same thing would be longer (the prefix of ERROR:filename) - unless there's a nice one-liner to do it, if there is, please do share!

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

I like bubbling up error messages in SystemError as well. Originally the messages where multi-line, this is why I separated them, but I'll change it back to just raising.

Btw. I don't understand the comment about this not being an error. You mean that this is a more serious thing? With this complexity it's only cosmetic, and ERROR is imho better to pick out/understand.

> +    except (subprocess.CalledProcessError, OSError) as e:
> +        logging.error("Problem encountered when running lsb_release: %s", e)
> +        raise SystemExit(1)
> +
> +
> +def trust_the_key(key: str) -> None:
> +    """
> +    Trust the key.
> +
> +    The key is trusted by running the apt-key command.
> +    """
> +    try:
> +        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 think the most common special char in URLs is '%' and slugifying it would unnecessarily obfuscate the source, this is why I rejected regex sub early on. I also like to dodge the regexes as much as possible to make it obvious for the next person :)

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

removed the first one;
renamed the second one to auth_file_path

> +    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')

yeah, makes sense

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

ditto

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

oops
done

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

great idea, thanks!

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