Create stronger run log cache abstraction

[williammartin]

Description

Since we made some changes to CacheDir in go-gh there are some places in gh which hard coded assumptions about where the cache dir was location. For example gh run view attempted to write files directly to os.TempDir() / gh-cli-cache. To fix this we expose the CacheDir through the cli's own config (in future, we may want to expose other dirs in the same way) and then inject that into the RunLogCache.

In doing this, we also strengthen the abstraction because previously callers of RunLogCache needed to provide it with a full file path for every operation instead of a key (which makes a lot more sense when there is an interface that could be implemented e.g. in memory). However, I continued to construct the file paths in the same way so that cache entries aren't expired unnecessarily.

Previously, the cache.Exists method was hiding possible error states that would later crop up in cache.Create by assuming all errors mean the cache doesn't exist (when for example, it could be missing permissions). I made the error a first class part of the method signature that must be handled.

Finally, I removed the testRunLogCache in the tests because as a stub it was still deeply coupled to the test setup (i.e. tests that mocked the return of ./fixtures/run_log.zip. It seemed better to remove this coupling and to get additional confidence in the RunLogCache (which was previously untested) by just using the real thing; the file system is fast and reliable. Since we don't have the test abstraction, we can remove the unused interface, it can always be returned later if at some point we feel we need it (unlikely?).

[andyfeller]

Is os.MkdirAll not needed anymore?

Since the cache directory path can change, it seems like we'd still want to make sure it exists before creating the run log cache, right?

[williammartin]

Yeh that's a fair point. Initially I thought that something would always create this earlier, but that in fact may not be true. I'll make sure to put this back.

[williammartin]

Added it back, thanks!

[williammartin]

Though I'm actually not sure what the permissions should be here now 🤔 It kind of depends where it is, which makes me think we're missing an abstraction.

[andyfeller]

So, this is a whole topic as https://groups.google.com/g/golang-dev/c/TXlDtwxaTcM?pli=1 highlights as different operating systems behave different like umask ... 🤔

[williammartin]

Specifically, if it lands in /tmp/gh-cli-cache then this is a shared dir that needs to be world writable. If it lands in $HOMEDIR/.cache/gh then it should only be writeable by the user. In the first case it could be /tmp/gh-cli-cache-$USER but I don't know if that's safe in all cases.

[andyfeller]

If we want to change the permissions used when a directory is created, let's make that a follow up issue as I imagine it is complicated as some users might be using gh across multiple users and want it more permissive versus more security minded folks wanting this more restrictive (0700)

[williammartin]

It's also probably not the end of the world to have 0755 because the directories above it will not be listable if they are configured correctly.

[andyfeller]

lgtm