Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

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

Already on GitHub?Sign in to your account

Merged
pawbana merged 21 commits intomainfrompb/terraform-install-test-flake-2
Nov 18, 2025

Conversation

@pawbana
Copy link
Contributor

@pawbanapawbana commentedNov 14, 2025•
edited
Loading

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

funcPersistentCacheDir(t*testing.T)string {

Fixes:coder/internal#1126

@pawbana
Copy link
ContributorAuthor

I think69021e7 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/terraform4.310s...=== FAIL: enterprise/coderd  (0.00s)FAILgithub.com/coder/coder/v2/enterprise/coderd [build failed]=== ErrorsError: enterprise\coderd\workspaces_test.go:3403:28: undefined: testutil.CacheTFProviders

Copy link
Member

@mtojekmtojek 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.


varerrbuf 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

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
ContributorAuthor

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
ContributorAuthor

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

I think69021e7 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
ContributorAuthor

I think69021e7 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).

@pawbanapawbana marked this pull request as draftNovember 17, 2025 10:41
@pawbanapawbana marked this pull request as ready for reviewNovember 17, 2025 15:35
@pawbanapawbana changed the titlefix: add missing os/arch mocks for terraform install testfix: add caching for terraform installer filesNov 17, 2025
@pawbanapawbana changed the titlefix: add caching for terraform installer filesfix: add cache for terraform installer filesNov 17, 2025
@pawbana
Copy link
ContributorAuthor

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)FAILgithub.com/coder/coder/v2/enterprise/coderd [build failed]=== ErrorsError: enterprise\coderd\workspaces_test.go:3403:28: undefined: testutil.CacheTFProvidersDONE 12046 tests, 131 skipped, 1 failure, 1 error in 426.864s

Copy link
Contributor

@hugodutkahugodutka left a comment

Choose a reason for hiding this comment

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

LGTM


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

@hugodutkahugodutkaNov 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)

@pawbanapawbana merged commit158243d intomainNov 18, 2025
30 checks passed
@pawbanapawbana deleted the pb/terraform-install-test-flake-2 branchNovember 18, 2025 08:52
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsNov 18, 2025
@mtojek
Copy link
Member

mtojek commentedNov 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 freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@hugodutkahugodutkahugodutka approved these changes

@mtojekmtojekAwaiting requested review from mtojek

@ssncferreirassncferreiraAwaiting requested review from ssncferreira

Assignees

@pawbanapawbana

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
[8]ページ先頭
©2009-2025 Movatter.jp