launchpad-reviewers team mailing list archive

[Ines Almeida <mp+473246@xxxxxxxxxxxxxxxxxx>]

This MP has a few conflicts that need addressing.

I also feel like the main commit message is lacking some context of what this MP adds, would be nice to add to it for posterity. I'd also perhaps phrase it as "Add run logic to RockRecipeRequestBuildsJob" instead of "Implement RockRecipeRequestBuildsJob" simply because it seems clearer to me of what's being changed.

Good stuff

Diff comments:

> diff --git a/lib/lp/rocks/adapters/buildarch.py b/lib/lp/rocks/adapters/buildarch.py
> index 7f0cd5a..d2a1569 100644
> --- a/lib/lp/rocks/adapters/buildarch.py
> +++ b/lib/lp/rocks/adapters/buildarch.py
> @@ -122,42 +122,17 @@ class RockBaseConfiguration:
>          return cls(build_on, run_on=run_on)
>  
>  
> -def determine_instances_to_build(
> -    rockcraft_data, supported_arches, default_distro_series
> -):
> +def determine_instances_to_build(rockcraft_data, supported_arches):

Why change this here from the past MP to this one? Why wasn't this changed in the past MP, and why do we want to have it be different from the charmcraft case?

>      """Return a list of instances to build based on rockcraft.yaml.
>  
>      :param rockcraft_data: A parsed rockcraft.yaml.
>      :param supported_arches: An ordered list of all `DistroArchSeries` that
>          we can create builds for.  Note that these may span multiple
>          `DistroSeries`.
> -    :param default_distro_series: The default `DistroSeries` to use if
> -        rockcraft.yaml does not explicitly declare any bases.
>      :return: A list of `DistroArchSeries`.
>      """
>      bases_list = rockcraft_data.get("bases")
> -
> -    if bases_list:
> -        configs = [
> -            RockBaseConfiguration.from_dict(item) for item in bases_list
> -        ]
> -    else:
> -        # If no bases are specified, build one for each supported
> -        # architecture for the default series.
> -        configs = [
> -            RockBaseConfiguration(
> -                [
> -                    RockBase(
> -                        default_distro_series.distribution.name,
> -                        default_distro_series.version,
> -                        das.architecturetag,
> -                    ),
> -                ]
> -            )
> -            for das in supported_arches
> -            if das.distroseries == default_distro_series
> -        ]
> -
> +    configs = [RockBaseConfiguration.from_dict(item) for item in bases_list]
>      # Ensure that multiple `run-on` items don't overlap; this is ambiguous
>      # and forbidden by rockcraft.
>      run_ons = Counter()
> diff --git a/lib/lp/rocks/interfaces/rockrecipe.py b/lib/lp/rocks/interfaces/rockrecipe.py
> index 18b5099..b803488 100644
> --- a/lib/lp/rocks/interfaces/rockrecipe.py
> +++ b/lib/lp/rocks/interfaces/rockrecipe.py
> @@ -6,10 +6,14 @@
>  __all__ = [
>      "BadRockRecipeSource",
>      "BadRockRecipeSearchContext",
> +<<<<<<< lib/lp/rocks/interfaces/rockrecipe.py

Need to fix these conflicts

>      "ROCK_RECIPE_ALLOW_CREATE",
>      "ROCK_RECIPE_PRIVATE_FEATURE_FLAG",
> -<<<<<<< lib/lp/rocks/interfaces/rockrecipe.py
>  =======
> +    "CannotFetchRockcraftYaml",
> +    "CannotParseRockcraftYaml",
> +    "ROCK_RECIPE_ALLOW_CREATE",
> +    "ROCK_RECIPE_PRIVATE_FEATURE_FLAG",
>      "RockRecipeBuildAlreadyPending",
>      "RockRecipeBuildDisallowedArchitecture",
>      "RockRecipeBuildRequestStatus",
> diff --git a/lib/lp/rocks/model/rockrecipe.py b/lib/lp/rocks/model/rockrecipe.py
> index 4ae64c7..a9de221 100644
> --- a/lib/lp/rocks/model/rockrecipe.py
> +++ b/lib/lp/rocks/model/rockrecipe.py
> @@ -472,6 +481,88 @@ class RockRecipe(StormBase):
>          )
>          return self.getBuildRequest(job.job_id)
>  
> +    def requestBuildsFromJob(
> +        self,
> +        build_request,
> +        channels=None,
> +        architectures=None,
> +        allow_failures=False,
> +        logger=None,
> +    ):
> +        """See `IRockRecipe`."""
> +        try:
> +            rockcraft_data = removeSecurityProxy(
> +                getUtility(IRockRecipeSet).getRockcraftYaml(self)
> +            )
> +
> +            # Sort by (Distribution.id, DistroSeries.id, Processor.id) for
> +            # determinism.  This is chosen to be a similar order as in
> +            # BinaryPackageBuildSet.createForSource, to minimize confusion.
> +            supported_arches = [
> +                das
> +                for das in sorted(
> +                    self.getAllowedArchitectures(),
> +                    key=attrgetter(
> +                        "distroseries.distribution.id",
> +                        "distroseries.id",
> +                        "processor.id",
> +                    ),
> +                )
> +                if (
> +                    architectures is None
> +                    or das.architecturetag in architectures
> +                )
> +            ]
> +            instances_to_build = determine_instances_to_build(
> +                rockcraft_data, supported_arches
> +            )
> +        except Exception as e:
> +            if not allow_failures:
> +                raise
> +            elif logger is not None:
> +                logger.exception(
> +                    " - %s/%s/%s: %s",
> +                    self.owner.name,
> +                    self.project.name,
> +                    self.name,
> +                    e,
> +                )
> +
> +        builds = []
> +        for das in instances_to_build:

`das` is not a clear variable name and `instances_to_build` doesn't help - it took me a bit to understand what it stood for (I assume is for distro-arch-series).  If you want to keep it as coherent with charms as possible, could we at least add a short comment?

> +            try:
> +                build = self.requestBuild(
> +                    build_request, das, channels=channels
> +                )
> +                if logger is not None:
> +                    logger.debug(
> +                        " - %s/%s/%s %s/%s/%s: Build requested.",
> +                        self.owner.name,
> +                        self.project.name,
> +                        self.name,
> +                        das.distroseries.distribution.name,
> +                        das.distroseries.name,
> +                        das.architecturetag,
> +                    )
> +                builds.append(build)
> +            except RockRecipeBuildAlreadyPending:
> +                pass
> +            except Exception as e:
> +                if not allow_failures:
> +                    raise
> +                elif logger is not None:
> +                    logger.exception(
> +                        " - %s/%s/%s %s/%s/%s: %s",
> +                        self.owner.name,
> +                        self.project.name,
> +                        self.name,
> +                        das.distroseries.distribution.name,
> +                        das.distroseries.name,
> +                        das.architecturetag,
> +                        e,
> +                    )
> +        return builds
> +
>      def getBuildRequest(self, job_id):
>          """See `IRockRecipe`."""
>          return RockRecipeBuildRequest(self, job_id)


-- 
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/473246
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/launchpad:implement-RockRecipeRequestsBuildsJob into launchpad:master.