fix(workflows): Disable Go cache for non-test jobs in AWS workflow

[candiduslynx]

Part of #9388

[erezrokah]

Thanks for the PR @candiduslynx, let's start by scoping this PR to a single change (the caching one).
Since CI changes can have a significant impact, we should make them as small as possible and test them one by one.

I'll try to test this change later on

[erezrokah]

Added a few more comments

[erezrokah]

Added another comment as it seems setup-go should already cache build artifacts so we should probably research why it doesn't work before opting out the default logic

[candiduslynx]

@erezrokah I think this might actually work for the AWS test cache, and then we can fix it in other workflows as well

[erezrokah]

Added a few more comments. I don't think we should disable the cache for the test policies job.

We should do something like:

      - uses: actions/cache@v3
        with:
          path: |
            ~/.cache/go-build
            ~/go/pkg/mod
          key: ${{ runner.os }}-go-1.19.5-policies-cache-${{ hashFiles('plugins/source/aws/go.sum') }}
          restore-keys: |
            ${{ runner.os }}-go-1.19.5-policies-cache-plugins-source-aws

[erezrokah]

Suggested change

cache: false

This already the default so not needed. We can do it once we update to v4

[candiduslynx]

I think it's better to leave this explicitly turned off as it doesn't affect the result now

[erezrokah]

Suggested change

cache: false

This already the default so not needed. We can do it once we update to v4

[candiduslynx]

I think it's better to leave this explicitly turned off as it doesn't affect the result now

[erezrokah]

What's the added value of setting a configuration to the default value?

[erezrokah]

Also related actions/setup-go#358

[candiduslynx]

Added a few more comments. I don't think we should disable the cache for the test policies job.

I don't think so, too.
However, currently I'd like to skip this in test-policies as it only runs the build.

The proper solution IMO would be to disable setup-go caching and use manual logic for caching:

• Save only from main after tests and do it regardless of the cache hit (this way we update the cache with the latest test data available)
• Other jobs (including releases as well) should only perform cache restore

[erezrokah]

The proper solution IMO would be to disable setup-go caching and use manual logic for caching:

• Save only from main after tests and do it regardless of the cache hit (this way we update the cache with the latest test data available)
• Other jobs (including releases as well) should only perform cache restore

I think this discussion is out of scope for this PR

However, currently I'd like to skip this in test-policies as it only runs the build.

What the reasoning for not caching test-policies? Won't that make the CI slower as each time we'll do a full build, impacting the performance improvement we're trying to achieve?

[erezrokah]

Also I think once actions/setup-go#358 is resolved we'll revert back to using setup-go caching

[candiduslynx]

What the reasoning for not caching test-policies? Won't that make the CI slower as each time we'll do a full build, impacting the performance improvement we're trying to achieve?

We are mostly hit by the testing part, not the build one. So, instead of introducing another cache I'd like to scope caching in setup-go only to the testing job

[erezrokah]

It takes ~6m to build AWS without cache:

And 8s with cache:

The results are from this PR.

Not sure we'd want to give that up

[erezrokah]

We are mostly hit by the testing part, not the build one

I think this is because caching didn't work for tests, and worked for builds

[erezrokah]

Suggested change

	uses: actions/setup-go@v3
	uses: erezrokah/setup-go@feat/add_cache_prefix
	with:
	go-version-file: plugins/source/aws/go.mod
	cache-key-prefix: test-cache-

And for policies we can use cache-key-prefix: policies-cache-.

Waiting for actions/setup-go#366 to be reviewed

[candiduslynx]

I think that we might be getting into another issue with this solution:

• the cache limit is 10 GiB
• each Go cache is about 100-300 MiB, let's use 150 MiB in the calculations to be on the conservative side (300 MiB likely comes from the huge plugins like AWS or Azure)
• that means we can store something like 50 caches to be used effectively before they start being evicted
• we already have 38 source plugins & 21 destination plugins (59 in total, add CLI to it to have 60)
• If we don't store a single cache but, instead, use 3 different caches, we effectively will store 150 (test) + 100 + 100 (2 x build & mod) = 350 MiB for a plugin (= 21 GB for all plugins + CLI, so we will be evicting a lot of data)

Looking at this, I suggest storing a single cache for plugin, that will include:

• mod cache
• build & test cache

To make most of it, we'll have to store it after the testing (and, better yet, from the main branch for it to be properly reused), and to restore in other builds related to a plugin (or CLI).

[erezrokah]

I suggest we first fix the bug in our CI, and do any cache optimization later on. Once we fix the bug we should be able so see if we're hitting any cache limits issues, the should be able to do benchmarks and see if that can be improved

[candiduslynx]

For the cache limits we can take a look here: https://github.com/cloudquery/cloudquery/actions/caches

I'll be updating the workflow files for the heavy sources (aws, azure, gcp, k8s) to work with go cache properly, we can discuss the different approach separately.

[erezrokah]

we already have 38 source plugins & 21 destination plugins (59 in total, add CLI to it to have 60)

We only need this fix for 4 plugins, the ones that have policy tests

[candiduslynx]

Drop this impl to start anew