launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+464639@xxxxxxxxxxxxxxxxxx>]

I just add there what we found during the peer programming session :)

Diff comments:

> diff --git a/lib/lp/snappy/interfaces/snapbuild.py b/lib/lp/snappy/interfaces/snapbuild.py
> index 52019bf..46bd205 100644
> --- a/lib/lp/snappy/interfaces/snapbuild.py
> +++ b/lib/lp/snappy/interfaces/snapbuild.py
> @@ -370,6 +371,14 @@ class ISnapBuildView(IPackageBuildView, IPrivacy):
>  
>          :return: A collection of URLs for this build."""
>  
> +    @export_operation_as("build_metadata_url")
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    def getBuildMetadataUrl():
> +        """URLs for all the files produced by this build.
> +
> +        :return: A collection of URLs for this build."""

Since we are returning None if the metadata is not present we should specify that in the docstring, I would say something like: Metadata file URL, None if the metadada file is not present.

> +
>  
>  class ISnapBuildEdit(IBuildFarmJobEdit):
>      """`ISnapBuild` attributes that require launchpad.Edit."""
> diff --git a/lib/lp/snappy/model/snapbuild.py b/lib/lp/snappy/model/snapbuild.py
> index df2e2c2..13a52b8 100644
> --- a/lib/lp/snappy/model/snapbuild.py
> +++ b/lib/lp/snappy/model/snapbuild.py
> @@ -425,6 +425,11 @@ class SnapBuild(PackageBuildMixin, StormBase):
>      def getFileUrls(self):
>          return [self.lfaUrl(lfa) for _, lfa, _ in self.getFiles()]
>  
> +    def getBuildMetadataUrl(self):
> +        for url in self.getFileUrls:

self.getFileUrls(), missing parentheses

> +            if url.endswith("metadata.json"):
> +                return url
> +
>      @cachedproperty
>      def eta(self):
>          """The datetime when the build job is estimated to complete.
> diff --git a/lib/lp/snappy/tests/test_snapbuild.py b/lib/lp/snappy/tests/test_snapbuild.py
> index d8b7758..edc917e 100644
> --- a/lib/lp/snappy/tests/test_snapbuild.py
> +++ b/lib/lp/snappy/tests/test_snapbuild.py
> @@ -1009,6 +1009,21 @@ class TestSnapBuildWebservice(TestCaseWithFactory):
>          for file_url in file_urls:
>              self.assertCanOpenRedirectedUrl(browser, file_url)
>  
> +    def test_getBuildMetadataUrl(self):
> +        # API clients can fetch the metadata from the build, generated by the
> +        # fetch service
> +        db_build = self.factory.makeSnapBuild(requester=self.person)
> +        metadata_file = self.factory.makeFileAlias(

s/self.factory.makeFileAlias/self.factory.makeLibraryFileAlias and wrap the file creation with:

```
with person_logged_in(self.person):
    ...
```

> +            content="some_json",
> +            filename="xxx-metadata.json",
> +        )
> +        db_build.addFile(metadata_file)
> +
> +        build_url = api_url(db_build)
> +        logout()
> +        response = self.webservice.named_get(build_url, "build_metadata_url")
> +        self.assertEndsWith(response.jsonBody(), "metadata.json")
> +
>  
>  class TestSnapBuildMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
>      """Test SnapBuild macaroon issuing and verification."""


-- 
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/464639
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/launchpad:expose-build-metadata-via-api into launchpad:master.