launchpad-reviewers team mailing list archive

[Ines Almeida <mp+464639@xxxxxxxxxxxxxxxxxx>]

I know this isn't tested, but just a initial review!

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

I think this will still be exported a method, and if so we usually go for camel case. Maybe we want to export it as a attribute instead?

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

Maybe it will be good return `None` if `use_fetch_service` is False?

> +            if url.endswith("metadata.json"):

If we do go for `{build_cookie}_metadata.json` we can actually check the full name of the file.

----------
We should add a comment here referencing the part in the code where `metadata.json` is defined (which will only be merged in my open MP). If this one is merged first, I'll add it on my side.

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

I think this test should have a couple more files created for the snap to make the test more meaningful, and maybe a negative test that returns `None` when the file isn't there (which will be the case for most snaps).

> +        # 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(
> +            content="some_json",
> +            filename="xxx-metadata.json",

In my WIP I had `xxx_metadata.json`, (with an underscore), I think it's best to keep it as close as possible, but we can also change from my side.

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