launchpad-reviewers team mailing list archive

[Quentin Debhi <mp+486190@xxxxxxxxxxxxxxxxxx>]

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(

yeah i remember seeing `self.rootpath.aql`
but is the `rootpath` an object?
when you see `config.`, it is something defined inside `lib/lp/services/config/schema-lazr.conf`, and these fields are set through deploy secrets/charm configs
this makes me think that `base_url` would just be a string

this confirmed by:
```
[artifactory]
# Base URL for publishing suitably-configured archives to Artifactory.
# example: https://canonical.example.com/artifactory
# datatype: string
base_url: none
```

so is that the right way to call the aql method? can you do `<str>.aql()`?
maybe you can and i just don't know how it works, but this seems weird to me

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

we won't use remote repos (if by remote you mean GitHub or somewhere else)
this publishing work is only happening with Launchpad stored repos

does that void your findings, im not certain, but if you find that `git_ref` is always reliable for Launchpad repos, then we should have a case for it

you should  just make sure that this is not duplicated behaviour ofc
for example, if `git_ref` is set only if `git_https_url` or `git_repository_url` is set, then it's pointless

hope im making sense here

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