tomcoldrick-ct
commented
4 years ago Nice work, obviously I might not be the best eyes on this given some of it is my doing. I've only had a brief skim thus far, but things look good.
A couple of larger points, mainly in the form of a brain dump, please tell me to elaborate if it's nonsensical:
- When I was whipping this together I was trying to get a proof of concept, if we're going to merge this I think we need to give more thought to how we configure this (this is entirely my fault). I wonder if we could move this behind the AssetStore API, and move asset store configuration to a section in the configuration file, maybe called
assetCache? So that is, we require anassetCachespecified in a config file, which can be one of:
- BlobAccess-based, as current AssetStore is, allows for local storage for folk without an action cache (e.g. a BuildStream remote cache only deployment)
- ActionCache-based, analogous to the work done already here
- None, which means no caching of values, and no support for
Push.
The Pusher then becomes a thin wrapper around this asset cache (but I still think this interface is worth creating, for consistency with Fetcher), and we can make a CachingFetcher that automatically wraps around this too. This might actually mean that Pusher configuration is not necessary for a user, as it should really be covered by the assetCache.
In my view this approach allows for nicer configuration of the caching behaviour, plus will simplify the signature of the New*FromConfiguration methods. Does this make sense?
-
The Directory-Tree conversion stuff is probably best placed in a separate module of its own, even if it's just a
util. It seems nifty, and might prove helpful for e.g. Completeness Checking (should we want to not just say "use the AC caching"). -
Not necessarily in this PR, but we should make the
Commands generated actually do what they're supposed to, i.e. download a remote asset. Doing this will probably require more thought, though, especially with the extensibility we need to cover from qualifiers. On a related note, if we're just throwing things in the AC as a list of URIs in theCommand.Arguments, we should prepend this with some clear identifier that this is generated from bb-remote-asset, lest things go wrong and the AC is just filled with actions full of URLs with no other indication what it's about. -
Given this is based on #7, I'd like that to be merged first, if you fancy giving that a review.
sdclarke
As outlined in issue #3, this PR will allow the asset hub to store assets in the action cache by converting Fetch/Push requests into actions.
This PR currently only checks/stores the first URI specified in the request, this should be changed to account for all of the URIs in the request before being merged.This has been added in the latest commit (909b90e)This PR removes the caching fetcher/pusher and the storage package from bb-remote-asset, as they will no longer be required and would require reworking due to bb-storage changes between when they were written and now.