search

buildbarn / bb-browser

Web frontend for exploring an Action Cache / Content Addressable Storage
Apache License 2.0
42 stars 35 forks source link

Support empty instance names #8

Closed edbaunton closed 5 years ago

edbaunton commented 5 years ago

It is valid for a remote execution service and CAS to use an empty instance name.

Unfortunately it seems somewhat ugly to add an optional path to gorilla so had to double up all the router registrations. Happy to factor this to be a bit prettier if desired.

EdSchouten commented 5 years ago

Hi Ed,

Sorry for not getting back to you about this pull request earlier. In retrospect I think I didn't give the case of empty instance names enough thought. I never use them, which is why this never affected me.

At some point in the not so distant future I want to replace Buildbarn's web UI by a Single Page Application (SPA). By using hash-style URLs (http://bb-browser/#action/instance/hash/size/) it'll be easier for us to support empty instance names. The browser won't discard empty pathname components from the anchor/fragment string.

With regards to your patch, my concern is that it adds potential ambiguity to the URL schemes supported by bb-browser. For example, for /tree/ the URL ends with a subdirectory name. I guess that can't be matched properly in the case of both empty and non-empty instance names.

For the time being, I think the least awful solution is to use a special replacement value for the empty instance name. For example, let /action/_/${hash}/${size} correspond with the empty instance. This will make it impossible to support the instance _, but that's likely not that bad for the time being.

edbaunton commented 5 years ago

That's certainly fine with me - I had considered doing that initially too. Easier to maintain and less ambiguous. Will update.

edbaunton commented 5 years ago

Updated to reflect the discussion.

edbaunton commented 5 years ago

Ah, @mhadjimichael noticed a problem with the way other parts of the site link to each other - so we need to apply the special mapping logic in multiple places.