launchpad-reviewers team mailing list archive

[Dan Ryan <mp+428406@xxxxxxxxxxxxxxxxxx>]

Review: Approve

one question about the config file access (do you need more error handling around that?)
but otherwise looks good to me

Diff comments:

> diff --git a/lib/lp/code/model/cibuildbehaviour.py b/lib/lp/code/model/cibuildbehaviour.py
> index 726bb80..a3e2439 100644
> --- a/lib/lp/code/model/cibuildbehaviour.py
> +++ b/lib/lp/code/model/cibuildbehaviour.py
> @@ -70,6 +72,21 @@ def build_apt_repositories(distribution_name: str) -> list:
>      return rv
>  
>  
> +def build_package_repositories(distribution_name: str) -> list:
> +    # - load package repository configuration lines from JSON Array
> +    # - replace authentication placeholder
> +    try:
> +        lines = config["cibuild." + distribution_name]["package_repositories"]

if `config` is a ConfigParser instance and the given distribution doesn't exist, won't this raise a KeyError?

> +    except NoSectionError:
> +        return []
> +    if lines is None:
> +        return []
> +    rv = []
> +    for line in json.loads(lines):
> +        rv.append(replace_auth_placeholder(line))
> +    return rv
> +
> +
>  def build_plugin_settings(distribution_name: str) -> dict:
>      # - load key/value pairs from JSON Object
>      # - replace authentication placeholder


-- 
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/428406
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/launchpad:converge-naming-for-package-repositories into launchpad:master.