Don't post to the download cache when environment caching

[jobovy]

There appears to be a problem when one uses both the download cache and the environment cache. In particular, when the environment cache is hit, the current version still tries to post to the download cache, which fails because it's not set up (and shouldn't be run). The faulty logic appears to be here:

provision-with-micromamba/index.js

Lines 305 to 308 in 5fe88d3

	// Save cache on workflow success
	if (inputs.cacheDownloads && !downloadCacheHit) {
	saveCacheOnPost(...downloadCacheArgs)
	}

which is inconsistent with where downloadCacheArgs is defined here:

provision-with-micromamba/index.js

Lines 275 to 281 in 5fe88d3

	if (!envCacheHit || inputs.cacheEnvAlwaysUpdate) {
	// Try to restore the download cache.
	if (inputs.cacheDownloads) {
	const key = inputs.cacheDownloadsKey || `${MAMBA_PLATFORM}-${process.arch} ${today()}`
	downloadCacheArgs = [PATHS.micromambaPkgs, `micromamba-pkgs ${key}`]
	downloadCacheHit = await tryRestoreCache(...downloadCacheArgs)
	}

leading to an error that downloadCacheArgs is not iterable.

This PR fixes this by making the logic used to decide whether to post to the download cache consistent with the earlier logic on whether or not to use/setup the download cache.

I also added a test. In doing that, I noticed that the way the other cache tests were written, both jobs in the test would use an earlier cache version because the cache already exists for the job that's supposed to setup the cache (when running on the same day, the default cache-key is the same). So I created a custom key that incorporates the github.ref and the attempt number, which I think should uniquely determine a workflow run. Now the first job in the sequence always creates a new cache that is then used by the second job.

[jobovy]

Not sure who the maintainers are... ping @beckermr @wolfv?

[wolfv]

Thanks for this. Maybe @jonashaag has a minute to check this PR? :)

[jonashaag]

Thanks for pinging, I can have a look next week. Any chance you can check if the problem remains in #45?

[jobovy]

Thanks for the reply @wolfv @jonashaag! I tried with #45, but the problem remains (I tested it here: https://github.com/jobovy/galpy/runs/6357270889?check_suite_focus=true).

[jonashaag]

Thanks for the fix and test! Looks correct. I wonder if we should introduce something like const shouldUpdateEnv = ... and const shouldSaveDownloadsCache = shouldUpdateEnv && ....

We should merge #45 first.

[jobovy]

Thanks for the review!

Thanks for the fix and test! Looks correct. I wonder if we should introduce something like const shouldUpdateEnv = ... and const shouldSaveDownloadsCache = shouldUpdateEnv && ....

Yes, I agree that this would be better. I've just changed the code in this vein (but only using a single variable, which I think is clear enough). Hopefully now that #45 is merged, this can be merged now.

[jonashaag]

@wolfv please approve actions (and maybe give me permissions to do that?)

[jobovy]

Thanks!