launchpad-reviewers team mailing list archive

[İlker Emre Koç <mp+486190@xxxxxxxxxxxxxxxxxx>]

I wasn't used to the tests for the recipes which is why I had to make it wait. I will write them now. Logging will also be added, I just wanted to make sure I was on the correct track first ^^.

A call would be great! I am open to it.

And I didnt know there was a WIP status, wow. Thanks!

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(

Frankly, I copied that section from an example code in "lib/lp/archivepublisher/artifactory.py" inside "ArtifactoryPool" class' "getAllArtifacts" method since we already use it all the time and, thus, proven to work. 

That "aql" call creates the query and posts it to Artifactory at the same time. So it is the only operation.

We prepend this with the base_url because artifactory's python API is constructed around "ArtifactoryPath" class, aql is one of its methods and the only way to access the Artifactory directly. I took the config.artifactory.base_url from a different code in Launchpad, and while I couldn't find where the literal value is located within the production environment, there were configuration files and other demonstrated codes that showed me we would have that value. But frankly, this one of the parts that definitely needs explicit testing.

You are right about the handling though, in fact not only should I add those, I even found a mistake, I will see what I can do.

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

I actually thought the same but wasn't sure if it would impact performance or anything. Now that I think again though, it was pretty silly of me, its a simple string split... Yeah you are right

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

I actually looked into this a bit too long, it may be possible, but I gave up as the way we handle git_refs differ immensely between remote and our own repos. GitRef and GitRefRemote is a bit too different than I could handle for this purpose.

For example, GitRef seemingly doesnt save a repository_url while GitRefRemote does. 

And our git_ref inside craft recipes is called in the same manner as I did here. git_ref is a property that calls _git_ref() which checks whether we have a launchpad repo or a remote one, and if it finds launchpad one it puts its GitRef, else it creates a GitRefRemote, which do not use the same interface for storing their repo_urls.

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

I will add them now, I wasn't sure if my approach was correct, or if there were any special considerations that would affect my approach in regards to errors. Currently, without catching, it is only there to make sure the file is closed so that it doesnt hog any resources.

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