Return an error on unlock failure

[grahamhar]

This attempts to fix Issue 23017

When the lock can't be released return the err even if there is no previous error with the current action. This allows faster failure in CI/CD systems. Without this failure to remove the lock would result in the failure happening on a subsequent plan or apply which slows down the feedback loop in automated systems.

[hashicorp-cla]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #25729 into master will increase coverage by 0.37%.
The diff coverage is n/a.

Impacted Files	Coverage Δ
internal/getproviders/http_mirror_source.go	0.00% <0.00%> (-71.43%)	⬇️
internal/getproviders/hash.go	0.00% <0.00%> (-58.34%)	⬇️
configs/hcl2shim/values.go	47.95% <0.00%> (-34.50%)	⬇️
terraform/eval_state_upgrade.go	45.65% <0.00%> (-17.51%)	⬇️
command/version.go	54.43% <0.00%> (-12.24%)	⬇️
internal/getproviders/legacy_lookup.go	42.42% <0.00%> (-8.86%)	⬇️
lang/funcs/cidr.go	77.50% <0.00%> (-7.12%)	⬇️
backend/local/backend_refresh.go	35.41% <0.00%> (-6.10%)	⬇️
command/hook_ui.go	66.36% <0.00%> (-6.04%)	⬇️
backend/local/backend_local.go	36.89% <0.00%> (-4.34%)	⬇️
... and 91 more

[mildwonkey]

Hi @grahamhar ! Thank you for submitting this, it's a subtle little bug!

I'd like to second @ZymoticB's suggested change - thank you for that, @ZymoticB - as it's an elegant simplification.

I'd also really like to see tests in this package. There aren't any now, and it's certainly a bigger task than you signed up for when you opened this PR, so I'm absolutely not going to require this from you for this PR. I'll take a look at this package and see if there's a relatively simple test I can suggest (or add myself, if you gave us access to push to your fork).

[grahamhar]

Hi @grahamhar ! Thank you for submitting this, it's a subtle little bug!

I'd like to second @ZymoticB's suggested change - thank you for that, @ZymoticB - as it's an elegant simplification.

I'd also really like to see tests in this package. There aren't any now, and it's certainly a bigger task than you signed up for when you opened this PR, so I'm absolutely not going to require this from you for this PR. I'll take a look at this package and see if there's a relatively simple test I can suggest (or add myself, if you gave us access to push to your fork).

Added the suggested change and given you access if you want to add test coverage at https://github.com/grahamhar/terraform

[mildwonkey]

[EDIT]: It just hit me that this is technically a breaking change, so I am going to label it as such and defer merging until the master branch switches to the 0.14 development cycle - that should be later this month. This conveniently gives me some extra time to add that test, too!

Thanks @grahamhar ! You were much quicker than I expected, so I will merge this as-is. I nearly have testing worked out, but it's rather ugly, so I'll need a little more time, though I have been able to validate your fix.

[grahamhar]

[EDIT]: It just hit me that this is technically a breaking change, so I am going to label it as such and defer merging until the master branch switches to the 0.14 development cycle - that should be later this month. This conveniently gives me some extra time to add that test, too!

Thanks @grahamhar ! You were much quicker than I expected, so I will merge this as-is. I nearly have testing worked out, but it's rather ugly, so I'll need a little more time, though I have been able to validate your fix.

I did expect that to be the case. Thanks for the quick review and looking forward to the release of 0.14 :)

[mildwonkey]

We just cut a 0.13 branch and can start merging work targeting 0.14 🎉

I pushed a test (and rebased) so we're good to go - thank you @grahamhar!

[ghost]

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.