launchpad-reviewers team mailing list archive

[Ines Almeida <mp+464312@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
> index a2b8f71..7b1c919 100644
> --- a/lib/lp/buildmaster/builderproxy.py
> +++ b/lib/lp/buildmaster/builderproxy.py
> @@ -87,6 +98,41 @@ class BuilderProxyMixin:
>          args["proxy_url"] = new_session["proxy_url"]
>          args["revocation_endpoint"] = new_session["revocation_endpoint"]
>  
> +    def endProxySession(self, upload_path):
> +        if not self.use_fetch_service:
> +            # No cleanup needed for the buidler proxy
> +            return
> +
> +        if self.proxy_service is None:
> +            # A session was never started
> +            return
> +
> +        if not isinstance(self.proxy_service, FetchService):
> +            # There shouldn't be any instance where this error is called,
> +            # yet it's best to be sure of the `self.proxy_service` type.
> +            raise ProxyServiceException(
> +                "Expected a `FetchService` session, but got a "
> +                f"`{self.proxy_service.__class__.__name__}` session instead."
> +            )
> +
> +        # Fetch session metadata from the proxy service and store in the same
> +        # path as other build files. If storing or retrieving the file fails,
> +        # they will raise an exception, meaning we don't close the session.
> +        # Sessions will be closed automatically within the Fetch Service after
> +        # a certain amount of time configured by its charm (default 6 hours).
> +        metadata = yield self.proxy_service.retrieveMetadataFromSession()
> +        file_path = os.path.join(
> +            upload_path, f"{self.build.build_cookie}_metadata.json"

> What is your reasoning to use the naming you chose?

This is certainly not set in stone and something I still wanted to check. I had the same question about whether to have a fixed name, and thought maybe it would be best to find some variable that would uniquely identify it.

The build_cookie is a unique identifier for the build job (str).

Let's have a chat about this, because I'd also like to define what the name of the file should be.

> +        )
> +        yield self._worker.process_pool.doWork(
> +            SaveAsFileCommand,
> +            content=metadata,
> +            path_to_write=file_path,
> +        )
> +
> +        # Once metadata is saved, we close the session (which deletes its data)
> +        yield self.proxy_service.endSession()
> +
>  
>  class IProxyService:
>      """Interface for Proxy Services - either FetchService or BuilderProxy."""
> diff --git a/lib/lp/buildmaster/downloader.py b/lib/lp/buildmaster/downloader.py
> index 7cf1c9b..55a13e7 100644
> --- a/lib/lp/buildmaster/downloader.py
> +++ b/lib/lp/buildmaster/downloader.py
> @@ -38,6 +40,17 @@ class DownloadCommand(amp.Command):
>      }
>  
>  
> +class SaveAsFileCommand(amp.Command):

Indeed, this one was mostly copied from the DownloadCommand, and I'm not that knowledgeable about this part of the code either.

The main difference is that the download command streams the data from a file to write to another, and in this one we already start with the data we want to save so we simply write. It didn't feel like I could just use the same command, but we can certainly see if we can with a few adjustments.

My plan is to get it working as is, test it, and then think about cleaning up and optimizing.

> +    arguments = [
> +        (b"content", amp.Unicode()),
> +        (b"path_to_write", amp.Unicode()),
> +    ]
> +    response: List[Tuple[bytes, amp.Argument]] = []
> +    errors = {
> +        RequestException: b"REQUEST_ERROR",
> +    }
> +
> +
>  class RequestFetchServiceSessionCommand(amp.Command):
>      """Fetch service API Command subclass
>  


-- 
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/464312
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:fetch-service-buildd-manager-end-session into launchpad:master.