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

cli: Only rewrite provider locks file if changed #28230

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

alisdair
Copy link
Contributor

If the provider locks have not changed, there is no need to rewrite the locks file. Preventing this needless rewrite should allow Terraform to operate in a read-only directory, so long as the provider requirements don't change.

Slightly related to #27630, but serving a different use case. The goal here is to make a smaller change which can be back-ported to 0.14, and still allow being able to run Terraform in a read-only directory to allow upgrade from 0.13 to 0.14.

If the provider locks have not changed, there is no need to rewrite the
locks file. Preventing this needless rewrite should allow Terraform to
operate in a read-only directory, so long as the provider requirements
don't change.
@alisdair alisdair added cli 0.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Mar 29, 2021
@alisdair alisdair requested a review from a team March 29, 2021 20:14
@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #28230 (786d992) into main (340543d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
command/init.go 56.90% <100.00%> (+0.50%) ⬆️
terraform/evaluate.go 52.47% <0.00%> (-0.42%) ⬇️
terraform/node_resource_plan.go 98.05% <0.00%> (+1.94%) ⬆️
communicator/communicator.go 80.70% <0.00%> (+3.50%) ⬆️

Copy link
Contributor Author

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

📝

// TODO: Check whether newLocks is different from previousLocks and mention
// in the UI if so. We should emit a different message if previousLocks was
// empty, because that indicates we were creating a lock file for the first
// time and so we need to introduce the user to the idea of it.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this TODO was out of date. Immediately above we already check if newLocks is different from previousLocks and output two different messages. The same applies to v0.14:

terraform/command/init.go

Lines 845 to 872 in 1c68b7b

if !newLocks.Equal(previousLocks) {
if previousLocks.Empty() {
// A change from empty to non-empty is special because it suggests
// we're running "terraform init" for the first time against a
// new configuration. In that case we'll take the opportunity to
// say a little about what the dependency lock file is, for new
// users or those who are upgrading from a previous Terraform
// version that didn't have dependency lock files.
c.Ui.Output(c.Colorize().Color(`
Terraform has created a lock file [bold].terraform.lock.hcl[reset] to record the provider
selections it made above. Include this file in your version control repository
so that Terraform can guarantee to make the same selections by default when
you run "terraform init" in the future.`))
} else {
c.Ui.Output(c.Colorize().Color(`
Terraform has made some changes to the provider dependency selections recorded
in the .terraform.lock.hcl file. Review those changes and commit them to your
version control system if they represent changes you intended to make.`))
}
}
// TODO: Check whether newLocks is different from previousLocks and mention
// in the UI if so. We should emit a different message if previousLocks was
// empty, because that indicates we were creating a lock file for the first
// time and so we need to introduce the user to the idea of it.
moreDiags = c.replaceLockedDependencies(newLocks)
diags = diags.Append(moreDiags)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I'm pretty sure I just saw the TODO, TODID it and then forgot to remove the comment afterwards.

@alisdair alisdair merged commit 4fb5056 into main Mar 30, 2021
@alisdair alisdair deleted the alisdair/only-rewrite-provider-locks-file-if-changed branch March 30, 2021 13:12
@ghost
Copy link

ghost commented Apr 30, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
0.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants