You need to include some sort of delimiter ordogfood: stuff will hash the same asdog: foodstuff

didn't we just check that the directory doesn't already exist? How could the file already exist if the directory doesn't?

You're right, this check is redundant. I'll remove it.

Providers are versioned, so I don't think each test or each version of each test needs its own unique mirror.

It results in multiple copies of the providers being downloaded on each system, and there doesn't seem to be any mechanism to clean up old versions of tests. The CI caches expire after 14 days, but people's personal development environments won't get cleaned up.

terraform providers mirror can be run multiple times on the same target directory and it will merge the results:

You can run terraform providers mirror again on an existing mirror directory to update it with new packages.

https://developer.hashicorp.com/terraform/cli/commands/providers/mirror

So, what do you think of doing two passes over the test cases: one serially that sets of the mirror with the totality of the modules we need, then a second, parallel pass that runs the tests?

Doing a serial pass is pretty slow if you haven't cached the providers beforehand. I just tried it, and it takes 27 seconds to cache the providers serially in my Coder workspace. In contrast, doing it in parallel only takes 5 seconds.

The cache size across all tests is currently around 70 MB - probably not a big deal. That said, it could balloon if someone starts iterating on a test locally. To avoid that, maybe we should add cleanup logic instead of doing a serial pass?

We already know the paths to provider directories based on test hashes, so at the start ofTestProvision, we could check which ones are needed and remove the rest.

Cleanup logic would also be fine here, so we don't blow up people's cache.

I don't think we're entirely out of the woods here. While the different paths on Windows vs Linux might mean that those caches are separate, different architectures on the same OS don't seem like they'd have a different cache version.

Also, the paths might not be so different on macOS vs Linux, for example.

Hmm, you're right about different architectures on the same OS, I haven't thought about that. I'll generate unique cache key prefixes based on os and architecture.

Can't we handle this use case by using explicitrestore andsave steps? I.e. we restore in all cases, but only save onmain?

https://github.com/actions/cache/blob/5a3ec84eff668545956fd18022155c47e93e2684/caching-strategies.md#saving-cache

I considered it but the GitHub Actions syntax makes it unwieldy. I wanted to package all the cache steps into a single action to avoid code duplication across the 5 workflows that need the cache. I tried doing it withrestore andsave, but found thatI cannot add adefer-likesave statement in a composite action. So I'd need to add 3 additional steps to each of the workflows: one to generate the cache keys, onerestore, onesave. That's a lot of code duplication - I believe the current approach is cleaner.

Forking the entire repo seems like a lot of code duplication! It's just better hidden, and means we have yet another fork to manage.

That's a good point. I'll refactor it to use the more verbose restore and save approach.

I don't get why we need to have this special case for PRs. If we are preventing PRs from saving to the cache anyway, why do we need to restrict only PRs from restoring last month's cache if this month is not available?

Also, why are we even bothering to match on month? If today's cache isn't available, doesn't the cache action automatically match the most recent available cache based on thekey-prefix alone?

This approach lets us reset the cache completely at the start of each month. We upload an updated cache daily, and each update is based on the previous day's cache. Without the monthly cut-off onmain, any file added to the cache would persist indefinitely. The monthly reset ensures that old or obsolete files do not linger in the cache forever.

If you implement cleanup in the test code, which I think is a good idea, then we'll only cache what we need daily, and a monthly reset won't be required.

I'm hesitant to get rid of the monthly clean-up for two reasons:

1. If other tests start using the cache and don't clean up properly after themselves, the cache will grow.
2. If a Terraform provider that one of the tests depends on becomes permanently unavailable, we won't be able to detect it without a cache reset.

I don't think matching on the month complicates the code too much, so I'd be for keeping it.