only cache specific module dirs

[lowlydba]

Fixes #19

I got a green build on my fork so I think it looks good 🤞🏻

[potatoqualitee]

gorgeous! let's see if this is all green on my sides too then ill just re-release it as 4.5

[potatoqualitee]

I'm seeing failures that aren't making sense to me -- do you have any idea? https://github.com/potatoqualitee/psmodulecache/actions

[briantist]

It looks to me like a poisoned cache, maybe related to this commit? https://github.com/potatoqualitee/psmodulecache/commit/7ca415c161bc834f1d8b923fa8badf4f74a478e4

Or perhaps not that one specifically, but the main clue is that a cache was saved with the key in question, but that cache appears to be empty: https://github.com/potatoqualitee/psmodulecache/runs/5527601933?check_suite_focus=true#step:3:34

Received 22 of 22 (100.0%), 0.0 MBs/sec Cache Size: ~0 MB (22 B) /usr/bin/tar --use-compress-program zstd -d -xf /home/runner/work/_temp/ac96369f-b482-41b3-8c55-7f1e35028667/cache.tzst -P -C /home/runner/work/psmodulecache/psmodulecache Cache restored successfully

So no modules were restored, and the module count then ends up being 1 (and not 0) because a version of Pester already exists on the runner.

---

This action should probably do a secondary check after the cache is restored to ensure that the modules it thinks it restored actually do exist, and then do the install in case that went wrong, and emit a warning.

Unfortunately, the cache action still lacks any way to clear or invalidate a cache other than by using a new key.

This is especially problematic in an action like this that entirely controls its own key, because a user who uses it and manages to bungle their own cache has no way to fix it except to fork and modify the key, or to stop all use for 7 days to wait for it to expire.

And even if the action ensures that an install was done on a bad cache load, the cache will still consider that an access, and it will continue to not expire it, so the poisoned cache will remain ostensibly forever, and the action will be slow(er) until the key changes and/or it isn't used for 7 days.

---

My suggestions are:

• add a parameter to the action that allows user-defined input to be a part of the cache key: this puts some control of the cache key in the hands of the user, so that they at least have the ability to bust their own caches, and as a bonus it gives them the ability to scope caches separately if they have workflows where they don't want to be sharing them.
• the above would let you fix poisoned caches in this repo's tests by changing that same value. I need to look at the tests more closely but a nice method might even be to use the workflow run ID so that the cache is re-populated uniquely on every run, and that it always calls the action again to check that the cache was actually used.
• To aid in the above, add a parameter that can control whether caching happens at all
• Ad further, add outputs to the action itself that report the cache key used, the cache hit status, and whether the cache was sufficient (the case where cache was hit but the returned modules were not right).

---

I might be able to put in a PR for this stuff soon, if it sounds right to you.

[potatoqualitee]

I love the new parameter idea! go for it, and set a default to the string default.

i like all of those, thank you

[briantist]

@potatoqualitee ,

I did start these changes, but I ran into some thorny issues around the action accepting multiple values for shell. Looking through to code, it doesn't seem like multiple shell values should work, because the code path to calculate the module directory (both before and after this PR) treats shell as a single value (unlike the other code paths), but I do see you have some tests for those and they appear to work... so I may have just not properly followed it all.

But in any case multiple shells, as currently written, makes it more difficult to implement the changes above and to test them. I could refactor that too, but then it's making a bigger change, or we could remove multiple-shell-in-a-single-run, which makes testing and development easier, but is a breaking change and a surprise for anyone using that... it's a fairly easy migration though (call the action once for each shell), and I'm not sure the cache as is even has the correct contents when invoked with multiple shells, so I'd actually lean toward that if you're open to it. I could see how that would be a non-starter though.

Anyway I got pulled away before I could fully look into multiple shells and how it's working now so maybe I just need to spend more time on it, I'll get back to my branch and take a look when I have a moment :)