launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+464312@xxxxxxxxxxxxxxxxxx>]

Thanks! Looks good so far!

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

buidler -> builder

> +            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 an example value of `self.build.build_cookie`?

I am thinking whether it is necessary to have a variable name, or whether we want to use a fixed name.

What is your reasoning to use the naming you chose?

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

I have not touched this part of the code base yet. So, what is the difference between this command and the existing download command? Or in other words, why do we need a new command?

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