Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cliconfig: Allow breaking the dependency lock file using the environment #32726

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

apparentlymart
Copy link
Contributor

Since it's already possible to activate the dependency lock file using an environment variable, we should allow opting in to it having broken behavior using the environment too.

It's kinda odd in retrospect that TF_PLUGIN_CACHE_DIR is the only setting we allow to be configured both in the environment and the CLI configuration. That means that the infrastructure for dealing with that situation was relatively immature here and so I did some light refactoring to make it unit-testable without actually modifying the test program's environment. To reduce the risk of that I wrote what might be considered an excessive amount of tests for such a small change.

Closes #32656.

@apparentlymart apparentlymart requested a review from a team February 21, 2023 22:05
@apparentlymart apparentlymart self-assigned this Feb 21, 2023
@apparentlymart apparentlymart added the 1.4-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Feb 21, 2023
@apparentlymart apparentlymart added this to the v1.4.0 milestone Feb 21, 2023
Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Do we need to document this anywhere, or are the release notes sufficient?

internal/command/cliconfig/cliconfig.go Outdated Show resolved Hide resolved
Since it's already possible to activate the dependency lock file using an
environment variable, we should allow opting in to it having broken
behavior using the environment too.

It's kinda odd in retrospect that TF_PLUGIN_CACHE_DIR is the only setting
we allow to be configured both in the environment and the CLI
configuration. That means that the infrastructure for dealing with that
situation was relatively immature here and so I did some light refactoring
to make it unit-testable without actually modifying the test program's
environment.
@apparentlymart
Copy link
Contributor Author

This setting (in both of its forms) will be mentioned in the v1.4 Upgrade Guide once it's written, but we don't have that yet so I skipped doing that here and will instead contribute some text to the later PR for the upgrade guide.

@apparentlymart apparentlymart merged commit a86cef4 into main Feb 22, 2023
@github-actions
Copy link
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@wyardley
Copy link

Thanks @apparentlymart

minamijoyo added a commit to minamijoyo/tfmigrate that referenced this pull request Mar 9, 2023
Starting from Terraform v1.4, launching terraform providers in the
acceptance test has been failing more frequently with a text file busy
error.

```
--- FAIL: TestAccMultiStateMigratorApplySimple (1.07s)
    multi_state_migrator_test.go:123: failed to run terraform init: failed to run command (exited 1): terraform init -input=false -no-color
        stdout:

        Initializing the backend...

        Successfully configured the backend "s3"! Terraform will automatically
        use this backend unless the backend configuration changes.

        Initializing provider plugins...
        - Finding latest version of hashicorp/null...
        - Installing hashicorp/null v3.2.1...

        stderr:

        Error: Failed to install provider

        Error while installing hashicorp/null v3.2.1: open
        /tmp/plugin-cache/registry.terraform.io/hashicorp/null/3.2.1/linux_amd64/terraform-provider-null_v3.2.1_x5:
        text file busy
```

After some investigation, I found Go's `os/exec.Cmd.Run()` does not wait
for the grandchild process to complete; from the point of view of
tfmigrate, the terraform command is the child process, and the provider
is the grandchild process.

golang/go#23019

If I understand correctly, this is not a Terraform issue and
theoretically should occur in versions older than v1.4; the changes in
v1.4 may have broken the balance of execution timing and made the test
very flaky. I experimented with inserting some sleep but could not get
the test to stabilize correctly. After trying various things, I found
that the test became stable by enabling the
`TF_PLUGIN_CACHE_MAY_BREAK_DEPENDENCY_LOCK_FILE` flag was introduced in
v1.4. This is an escape hatch to revert to the v1.3 equivalent of the
global cache behavior change in v1.4.
hashicorp/terraform#32726

This behavior change has already been addressed in the previous commit
using a local file system mirror, so activating this flag does not seem
to make any sense. Even though I have no other reasonable solutions now,
please let me know if anyone finds a better solution.
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.4-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged cli enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting plugin cache force via env var
3 participants