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

internal/plugintest: Set CHECKPOINT_DISABLE=1 in terraform-exec directly #913

Merged
merged 2 commits into from Mar 28, 2022

Conversation

bflad
Copy link
Member

@bflad bflad commented Mar 23, 2022

Closes #684

@bflad bflad added technical-debt subsystem/tests Issues and feature requests related to the testing framework. labels Mar 23, 2022
@bflad bflad added this to the v2.13.0 milestone Mar 23, 2022
@bflad bflad requested a review from a team as a code owner March 23, 2022 19:05
Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

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

LG but I do have a question: why?

I looked at the linked issue, and it doesn't explain why we are removing this, how it came to be in the SDK, and why there shouldn't be calls in the plugin.go to disable it, while it's ok in the plugintest module.

@bflad
Copy link
Member Author

bflad commented Mar 28, 2022

Terraform CLI performs HTTP call(s) to checkpoint.hashicorp.com to verify that you're running the latest version of Terraform CLI when you execute some (all?) commands. Additional documentation about this feature can be found on the Terraform website. Here's the original issue for its removal.

This removes the extraneous HTTP call since:

  • The output is not visible at all to developers when running testing
  • When explicitly running testing against versions of Terraform CLI via TF_ACC_TERRAFORM_PATH or TF_ACC_TERRAFORM_VERSION, this check may be purposefully irrelevant as the intention was to run a non-latest version
  • Less networking calls the better as Checkpoint call failures could unnecessarily fail testing runs
  • Developers may be working in environments that require explicit egress, so no sense making this call if it doesn't provide any particular value

@bflad bflad merged commit 1c932cf into main Mar 28, 2022
@bflad bflad deleted the bflad-internal-plugintest-setenv branch March 28, 2022 13:37
@detro
Copy link
Contributor

detro commented Mar 28, 2022

Thank you, that is super helpful and makes perfect sense.

@github-actions
Copy link

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 Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
subsystem/tests Issues and feature requests related to the testing framework. technical-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove wd.Setenv and wd.Unsetenv
2 participants