launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+481408@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py b/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py
> index 0fc235a..a6ed921 100644
> --- a/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py
> +++ b/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py
> @@ -53,6 +53,96 @@ def generate_exclude_ppas_options(excluded_ppas):
>      return f" --exclude {' --exclude '.join(excluded_ppas)}"
>  
>  
> +def validate_parallel_publisher_config(parallel_publisher_config):
> +    """
> +    Validates whether the parallel publisher config follows the following
> +    rules:
> +    1. parallel run IDs are unique.
> +    2. only excluded ppas are used in parallel runs
> +    3. all required config values are present
> +    """
> +    runs = parallel_publisher_config.get("runs")
> +    if not runs:
> +        return True
> +    excluded_ppas = set(parallel_publisher_config.get("excluded_ppas"))
> +
> +    # runs are configured but there are no excluded PPAs
> +    if not excluded_ppas:
> +        hookenv.log(
> +            "Parallel publisher runs are configured but there are no PPAs"
> +            " excluded."
> +        )
> +        return False
> +
> +    run_ids = set()
> +    for run in runs:
> +        distro = run.get("distro")
> +        run_id = run.get("id")
> +        ppas = run.get("ppas")
> +
> +        if not ppas:
> +            hookenv.log("No PPAs specified for the run.")
> +            return False
> +
> +        if not distro:
> +            hookenv.log("No distribution specified for the run.")
> +            return False
> +
> +        if not run_id:
> +            hookenv.log("No run ID specified for the run.")
> +            return False
> +
> +        if run_id in run_ids:
> +            hookenv.log(f"Duplicate run ID found: {run_id}")
> +            return False
> +
> +        run_ids.add(run_id)
> +
> +        for ppa in ppas:
> +            if ppa not in excluded_ppas:
> +                hookenv.log(f"PPA {ppa} is not in the excluded PPAs list.")
> +                return False
> +    return True
> +
> +
> +def generate_parallel_publisher_cron_configs(parallel_publisher_config):
> +    if not parallel_publisher_config:
> +        return []
> +    runs = parallel_publisher_config.get("runs")
> +
> +    if not runs:
> +        return []
> +
> +    if not validate_parallel_publisher_config(parallel_publisher_config):

If the configuration is not valid, I would prefer to set an error status on the hook saying that there is some problem with the configuration, putting the unit in an error status

> +        return []
> +
> +    cron_configs = []
> +    for run in runs:
> +        distro = run.get("distro")
> +        run_id = run.get("id")
> +        ppas = run.get("ppas")
> +
> +        # ensure we have all the required config
> +        if not distro or not run_id or not ppas:
> +            hookenv.log(
> +                "Please specify each of distro, id, and ppas for parallel run."
> +                f" Got distro: {distro}, id: {run_id}, ppas: {ppas}"
> +            )
> +            continue
> +
> +        config = {}
> +        config["distro_option"] = (
> +            "--all-derived" if distro == "all_derived" else f"-d {distro}"
> +        )
> +        config["archive_options"] = [f"--archive {ppa}" for ppa in ppas]
> +        config["lockfilename_option"] = f"--lockfilename {run_id}.lock"
> +        config["logfilename"] = f"cron.ppa.{run_id}.log"
> +
> +        cron_configs.append(config)
> +
> +    return cron_configs
> +
> +
>  @when(
>      "launchpad.db.configured",
>      "memcache.available",


-- 
https://code.launchpad.net/~tushar5526/launchpad/+git/launchpad/+merge/481408
Your team Launchpad code reviewers is requested to review the proposed merge of ~tushar5526/launchpad:refactor-excluded-ppas-and-add-support-parallel-runs into launchpad:master.