sts-sponsors team mailing list archive

[Alberto Donato <mp+437533@xxxxxxxxxxxxxxxxxx>]

Review: Approve

nice, +1

some minor nits inline

Diff comments:

> diff --git a/maas_release_tools/launchpad.py b/maas_release_tools/launchpad.py
> index 7ea3191..175b0ab 100644
> --- a/maas_release_tools/launchpad.py
> +++ b/maas_release_tools/launchpad.py
> @@ -50,6 +55,19 @@ class LaunchpadActions:
>          """Return the MAAS user from LP."""
>          return self.lp.people[MAAS_USER]
>  
> +    def get_ppa(self, owner, name: str):
> +        """Return a reference to ppa:<owner>/<name>"""
> +        try:
> +            ppa = owner.getPPAByName(name=name)
> +        except NotFound:
> +            return None
> +        else:
> +            return ppa

this could be just

try:
   return owner.getPPAByName(name=name)
except NotFound:
   return None

> +
> +    def get_ppa_dependecies(self, ppa) -> list[str]:
> +        """Return a list this ppa dependecies"""
> +        return [d.dependency.name for d in ppa.dependencies]
> +
>      def snap_builder_exist(self, builder_name: str) -> bool:
>          try:
>              _ = self.lp.snaps.getByName(name=builder_name, owner=self.maas)
> diff --git a/maas_release_tools/makefile.py b/maas_release_tools/makefile.py
> new file mode 100644
> index 0000000..49a8145
> --- /dev/null
> +++ b/maas_release_tools/makefile.py
> @@ -0,0 +1,49 @@
> +"""Git helpers."""
> +
> +import re
> +import subprocess
> +from typing import NamedTuple, Optional
> +
> +_MAKE_VAR_RE = re.compile(r"(?P<name>.+)[ \t]?=[ \t]?(?P<value>.+)$")

I think you can use \s instead of [ \t]. Maybe it should actually be \s* (since you could have more  than one space)

> +
> +
> +class MakeResult(NamedTuple):
> +    output: str
> +    error: str
> +    code: int
> +
> +    @property
> +    def succeeded(self) -> bool:
> +        return self.code == 0
> +
> +
> +class Makefile:
> +    """A wrapper around the GNU make."""
> +
> +    def __init__(self, cwd: Optional[str] = None) -> None:

FWIW we can use str | None instead of Optional (same below).

> +        self.cwd = cwd
> +
> +    def get_ppa_path(self) -> Optional[str]:
> +        """Return the short revision for a reference."""
> +        result = self._run("print-MAAS_PPA")
> +        if not result.succeeded:
> +            return None
> +        match = _MAKE_VAR_RE.match(result.output)
> +        if not match:
> +            return None
> +        entry = match.groupdict()
> +        return entry["value"]
> +
> +    def _run(self, *args) -> MakeResult:
> +        proc = subprocess.run(
> +            ["make", *args],
> +            stdout=subprocess.PIPE,
> +            stderr=subprocess.PIPE,
> +            text=True,
> +            cwd=self.cwd,
> +        )
> +        return MakeResult(
> +            output=proc.stdout.strip(),
> +            error=proc.stderr.strip(),
> +            code=proc.returncode,
> +        )
> diff --git a/maas_release_tools/scripts/release_status.py b/maas_release_tools/scripts/release_status.py
> index 58c9b5f..c4b8a6b 100644
> --- a/maas_release_tools/scripts/release_status.py
> +++ b/maas_release_tools/scripts/release_status.py
> @@ -314,6 +330,35 @@ class MAASVersion(ReleaseStep):
>          ]
>  
>  
> +class MakefileUpdated(ReleaseStep):
> +    def __init__(self, preparer: ReleasePreparer, cwd: Optional[str] = None):
> +        super().__init__(preparer, cwd)
> +        ppa_name = self.preparer.version.major
> +        ppa_owner = self.launchpad.lp.people["maas"]
> +        self.ppa_path = f"ppa:{ppa_owner.name}/{ppa_name}-next"
> +
> +    @property
> +    def title(self) -> str:
> +        return "MAAS makefile updated"
> +
> +    def skip(self) -> bool:
> +        return self.preparer.version.grade in ("alpha", "beta")
> +
> +    def check(self) -> Tuple[bool, str]:

s/Tuple/tuple/

> +        make_ppa_path = self.make.get_ppa_path()
> +        return (
> +            make_ppa_path == self.ppa_path,
> +            f"MAAS_PPA set to `{make_ppa_path}`",
> +        )
> +
> +    def fix(self, doit: bool = False) -> Tuple[bool, list[str]]:
> +        return False, [
> +            "Open the Makefile file",
> +            f"Set `MAAS_PPA` to {self.ppa_path}",
> +            "Commit and push your changes",
> +        ]
> +
> +
>  class SnapTrack(ReleaseStep):
>      def __init__(self, preparer, snap_name):
>          super().__init__(preparer)
> @@ -427,22 +481,33 @@ class MAASPPA(ReleaseStep):
>              )
>          return True, None
>  
> +    def how_to_create_ppa(self, ppa_owner: str, ppa_name: str) -> list[str]:
> +        return [
> +            f"Go to https://launchpad.net/~{ppa_owner}/+activate-ppa";,
> +            f"Create '{ppa_name}' PPA",
> +            "Click `Change Details`",
> +            f"Enable the following processors: {', '.join(sorted(set(BUILD_ARCHS)))}",

s/processors/architectures/

> +        ]
> +
>      def fix(self, doit: bool = False) -> Tuple[bool, list[str]]:
> -        steps = []
>          if self.ppa is None:
> -            steps.append(
> -                f"Go to https://launchpad.net/~{self.ppa_owner.name}/+activate-ppa";
> +            return False, self.how_to_create_ppa(
> +                self.ppa_owner.name, self.ppa_name
>              )
> -            steps.append(f"Create '{self.ppa_name}' PPA")
>          else:
> -            steps.append(
> -                f"Go to https://launchpad.net/~{self.ppa_owner.name}/+archive/ubuntu/{self.ppa_name}/";
> -            )
> -        steps.append("Click `Change Details`")
> -        steps.append(
> -            f"Enable the following processors: {', '.join(sorted(set(BUILD_ARCHS)))}"
> -        )
> -        return False, steps
> +            steps = [
> +                f"Go to {self.ppa.web_link}/+edit",
> +                f"Enable the following processors: {', '.join(sorted(set(BUILD_ARCHS)))}",

same here

> +                "Save",
> +            ]
> +            if self.deps is not None:
> +                steps.extend(
> +                    [
> +                        f"Go to {self.ppa.web_link}/+edit-dependencies",
> +                        f"Add {', '.join(self.deps)} to Dependencies list",
> +                    ]
> +                )
> +            return False, steps
>  
>  
>  class MAASPackagePublished(MAASPPA):
> @@ -846,7 +993,7 @@ class MilestoneExist(ReleaseStep):
>      def check(self):
>          tag_name = self.preparer.version.version
>          try:
> -            self.preparer.launchpad._get_milestone(tag_name)
> +            self.launchpad._get_milestone(tag_name)

maybe it'd be worth to make the method public at this point

>          except UnknownLaunchpadEntry:
>              return (
>                  False,


-- 
https://code.launchpad.net/~alexsander-souza/maas/+git/maas-release-tools/+merge/437533
Your team MAAS Committers is subscribed to branch ~maas-committers/maas/+git/maas-release-tools:main.