launchpad-reviewers team mailing list archive

[Quentin Debhi <mp+486190@xxxxxxxxxxxxxxxxxx>]

make sure to add tests along the way so it's easier for you
logging can be useful too on failed operations (can't load the yaml, can't find an artifact etc...), i have put some inline comments for this

maybe a call is needed so i can grasp what you're trying to do

also setting the status to WIP

Diff comments:

> diff --git a/lib/lp/crafts/model/craftrecipebuildjob.py b/lib/lp/crafts/model/craftrecipebuildjob.py
> index 8133aeb..11b9f1c 100644
> --- a/lib/lp/crafts/model/craftrecipebuildjob.py
> +++ b/lib/lp/crafts/model/craftrecipebuildjob.py
> @@ -488,3 +498,69 @@ class CraftPublishingJob(CraftRecipeBuildJobDerived):
>  
>          # Combine all parts
>          return header + schema + servers + profiles + active_profiles
> +
> +    def _publish_properties(
> +        self, publish_url: str, artifact_name: str
> +    ) -> None:
> +        """Publish properties to the artifact in Artifactory."""
> +
> +        new_properties = {}
> +
> +        new_properties["soss.commit_id"] = self.build.revision_id
> +        new_properties["soss.source_url"] = self._recipe_git_url()
> +        new_properties["soss.type"] = "source"
> +        new_properties["soss.license"] = self._get_license_metadata()
> +
> +        artifact = config.artifactory.base_url.aql(

im not sure i understand what this is?
are you constructing the AQL query to use for later?
if so, why is it pre-pended with `config.artifactory.base_url`?

i don't see any call to artifactory there right? or is that the aql call? because this doesnt seem right, maybe you can explain it to me when you have time

when you call artifactory, make sure to handle the results robustly, including multiple/no hits

> +            "items.find",
> +            {
> +                "repo": self._extract_repository_name(publish_url),
> +                "name": artifact_name,
> +            },
> +            ".include",
> +            # We don't use "repo", but the AQL documentation says that
> +            # non-admin users must include all of "name", "repo",
> +            # and "path" in the include directive.
> +            ["repo", "path", "name"],
> +        )
> +
> +        path = ArtifactoryPath(artifact["path"], artifact["name"])
> +        path.set_properties(new_properties)
> +
> +    def _extract_repository_name(self, url: str) -> str:
> +        # Remove trailing slash if present
> +        url = url.rstrip("/")
> +
> +        # Use regex to match the last part after the last slash
> +        match = re.search(r"/([^/]+)$", url)

if the repo name is always the last part of the url, might aswell just split on the last '/', regex seem overkill here

> +
> +        if match:
> +            return match.group(1)
> +        else:
> +            raise Exception(
> +                "Couldn't parse repository name from publishing URL"
> +            )
> +
> +    def _recipe_git_url(self) -> str | None:
> +        """Get the recipe git URL."""
> +
> +        craft_recipe = self.build.recipe
> +        if craft_recipe.git_repository is not None:
> +            return craft_recipe.git_repository.git_https_url
> +        elif craft_recipe.git_repository_url is not None:
> +            return craft_recipe.git_repository_url
> +        else:

maybe we can reconstruct it from git_ref if thats the only field available?
not sure if it is possible to have `git_ref` set with no `recipe.git_repository.git_https_url`, maybe something to check?

> +            return None
> +
> +    def _get_license_metadata(self) -> str:
> +        """Get the license metadata from the build files."""
> +        for _, lfa, _ in self.build.getFiles():
> +            if lfa.filename == "metadata.yaml":
> +                lfa.open()
> +                try:
> +                    content = lfa.read().decode("utf-8")
> +                    metadata = yaml.safe_load(content)

if the load fails it should be handled, i dont see any catching here

> +                    return metadata.get("license")
> +                finally:
> +                    lfa.close()
> +        return "unknown"


-- 
https://code.launchpad.net/~ilkeremrekoc/launchpad/+git/launchpad/+merge/486190
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilkeremrekoc/launchpad:maven-cargo-metadata into launchpad:master.