launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+473246@xxxxxxxxxxxxxxxxxx>]

Thanks for the feedback!

I hope I was able to address all your concerns. I will also update the commit message when I rebase.

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

Only when I created this MP, I learned that only the "first gen" charm builds support builds without specifying a base, as there was a fallback to a default distro series.

I def. agree that this could have been a separate commit or MP, but as this only manifested with these changes, as a test failed, I fixed it here.

I am aware this is not best practice, but I needed to go for the shortcut to be able move  forward with the required pace.

>      """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

These will be fixed once I rebase to the previous branch, which I want to do once the one before the previous one got merged. I am aware this is not ideal, but constantly rebasing a chain of 10 MPs seems to be a time sink.

>      "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:

As much as I disliked the abbreviations used in Launchpad's codebase when I started, I somewhen got comfortable using them. `das` is a very common one, with more than 300 hits in the sourcecode. I do not think a comment in one of the 300 places will return a lot of value. We should have a discussion with the team, maybe at the retro?, about whether we should use full variable names, or whether we should introduce an index of abbreviations. I added an item for the next retro.

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