Skip to content

fix: add cache for terraform installer files #20776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
pawbana merged 21 commits into from
Nov 18, 2025
Merged

fix: add cache for terraform installer files #20776

pawbana merged 21 commits into from
Nov 18, 2025

Conversation

@pawbana
Copy link
Contributor

@pawbana pawbana commented Nov 14, 2025 •
edited
Loading

Replaces mocks by simple proxy that caches terraform files using test cache

coder/testutil/cache.go

Line 13 in 16b8e60

func PersistentCacheDir(t *testing.T) string {

Fixes: coder/internal#1126

@pawbana pawbana requested a review from hugodutka November 14, 2025 11:53
mtojek
mtojek reviewed Nov 14, 2025
View reviewed changes
mtojek
mtojek reviewed Nov 14, 2025
View reviewed changes
@pawbana
Copy link
Contributor Author

pawbana commented Nov 14, 2025

I think 69021e7 works although it is quite ugly: https://github.com/coder/coder/actions/runs/19376939370/job/55446613426 (some other test fails)

ok  	github.com/coder/coder/v2/provisioner/terraform	4.310s

...

=== FAIL: enterprise/coderd  (0.00s)
FAIL	github.com/coder/coder/v2/enterprise/coderd [build failed]

=== Errors
Error: enterprise\coderd\workspaces_test.go:3403:28: undefined: testutil.CacheTFProviders

mtojek
mtojek reviewed Nov 15, 2025
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you skipped some of comments from past iterations.

provisioner/terraform/install_test.go Outdated

var errbuf strings.Builder
if _, err := os.Stat(filepath.Join(tmpDir, "go.mod")); errors.Is(err, os.ErrNotExist) {
cmd := exec.Command("go", "mod", "init", "fake-terraform")
Copy link
Member

@mtojek mtojek Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking loud -

Since you have logic to dynamically create a fake terraform binary, can we use it for all cases, I mean mac/linux/windows?
Also, if the binary runs on the same platform as testing code, can it dynamically figure out the os and arch?

I'm curious if we can get rid of those if-windows clauses in favor of generic code for all platforms.

Copy link
Contributor Author

@pawbana pawbana Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that, although I'd rather avoid compiling if possible. I'd like to check if there is no other way for windows.
If compilation was windows is required then still I think it may be better to avoid it for linux/mac.

Copy link
Contributor Author

@pawbana pawbana Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @hugodutka a bit and decided to go with cached proxy approach. Installers are cached in test cache and only downloaded on cache miss.

@mtojek
Copy link
Member

mtojek commented Nov 15, 2025

I think 69021e7 works although it is quite ugly: https://github.com/coder/coder/actions/runs/19376939370/job/55446613426 (some other test fails)

@pawbana Hmm.. I admit I can't figure out the fix from the commit history. What was the root cause?

@pawbana
Copy link
Contributor Author

pawbana commented Nov 17, 2025

I think 69021e7 works although it is quite ugly: https://github.com/coder/coder/actions/runs/19376939370/job/55446613426 (some other test fails)

@pawbana Hmm.. I admit I can't figure out the fix from the commit history. What was the root cause?

Root cause is my first attempt to mock terraform installation files and not being aware I should also prepare mac/windows versions: edf056b (I've asked if I should revert it but was told it is fine to leave it failing for short time).

@pawbana pawbana marked this pull request as draft November 17, 2025 10:41
@pawbana pawbana marked this pull request as ready for review November 17, 2025 15:35
@pawbana pawbana changed the title fix: add missing os/arch mocks for terraform install test fix: add caching for terraform installer files Nov 17, 2025
@pawbana pawbana changed the title fix: add caching for terraform installer files fix: add cache for terraform installer files Nov 17, 2025
@pawbana
Copy link
Contributor Author

pawbana commented Nov 17, 2025

nightly-gauntlet fails due to unrelated reason but test passes: https://github.com/coder/coder/actions/runs/19435271482/job/55604365354#step:11:325

=== Failed
=== FAIL: enterprise/coderd  (0.00s)
FAIL	github.com/coder/coder/v2/enterprise/coderd [build failed]

=== Errors
Error: enterprise\coderd\workspaces_test.go:3403:28: undefined: testutil.CacheTFProviders


DONE 12046 tests, 131 skipped, 1 failure, 1 error in 426.864s

hugodutka
hugodutka approved these changes Nov 17, 2025
View reviewed changes
Copy link
Contributor

@hugodutka hugodutka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

provisioner/terraform/install_test.go

tmpDir := createFakeTerraformInstallationFiles(t)
addr := startFakeTerraformServer(t, tmpDir)
proxy := persistentlyCachedProxy(t)
Copy link
Contributor

@hugodutka hugodutka Nov 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
proxy := persistentlyCachedProxy(t)
// lets us cache terraform binaries between test runs to improve CI flakiness
proxy := persistentlyCachedProxy(t)

hugodutka
hugodutka reviewed Nov 17, 2025
View reviewed changes
@pawbana pawbana merged commit 158243d into main Nov 18, 2025
30 checks passed
@pawbana pawbana deleted the pb/terraform-install-test-flake-2 branch November 18, 2025 08:52
@github-actions github-actions bot locked and limited conversation to collaborators Nov 18, 2025
@mtojek
Copy link
Member

mtojek commented Nov 18, 2025 •
edited
Loading

I see you have already merged the PR. Next time please give reviewers more time to gain more 👍, unless it is emergency.

I can’t review it the regular way as Github fails with “Issue is locked”, so here is my review:

  1. Why do we need WriteTimeout and ReadTimeout? Why 30 sec?
  2. You can use RWMutex instead, and just lock the server when actually modifying the cache directory.
  3. Can we determine terraform version and download the terraform binary before running the test? No need to fiddle with mutexes.

BTW, these can be improved in follow-ups.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hugodutka hugodutka hugodutka approved these changes

@mtojek mtojek Awaiting requested review from mtojek

@ssncferreira ssncferreira Awaiting requested review from ssncferreira

Assignees

@pawbana pawbana

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

flake: TestInstall (provisioner/terraform) on macOS arm64

4 participants

@pawbana @mtojek @hugodutka