Ignore existing package hashes for providers lock command

[liamcervante]

This PR addresses part of the concerns raised in #29958

First, the terraform providers lock command will now ignore any existing hashes in the .terraform.lock.hcl file. It will retrieve new hashes as if there were none already logged and then merge any new hashes into the existing list. There is now no validation of any existing hashes. It does this with the introduction of the InstallNewProvidersForce InstallMode, which is set within providers_lock.go.

Second, the output of the terraform providers lock has been made more verbose. There are three cases tracked for each (provider,platform) pair requested:

1. There were existing checksums, and nothing new was found or recorded.
2. There were existing checksums, and new checksums were found and recorded.
3. There were no existing checksums, and new checksums have been found and recorded.

Full example output:

- Fetching hashicorp/google 4.27.0 for darwin_arm64...
- Retrieved hashicorp/google 4.27.0 for darwin_arm64 (signed by HashiCorp)
- Fetching hashicorp/null 3.1.0 for darwin_arm64...
- Retrieved hashicorp/null 3.1.0 for darwin_arm64 (signed by HashiCorp)
- Fetching hashicorp/aws 4.21.0 for darwin_arm64...
- Retrieved hashicorp/aws 4.21.0 for darwin_arm64 (signed by HashiCorp)
- Obtained hashicorp/null checksums for darwin_arm64; All checksums for this platform were already tracked in the lock file
- Obtained hashicorp/aws checksums for darwin_arm64; Additional checksums for this platform are now tracked in the lock file
- Obtained hashicorp/google checksums for darwin_arm64; This was a new provider and the checksums for this platform are now tracked in the lock file

Success! Terraform has updated the lock file.

Review the changes in .terraform.lock.hcl and then commit to your
version control system to retain the new checksums.

[apparentlymart]

Overall this looks like a good approach to me! I just had one question inline where I wasn't sure if I was understanding a particular part of the logic correctly.

My uncertainty there also made me think about how it might've given me a little more to go on if our TestProvidersLock also included some testing that the command's output contains the particular messages we're expecting, so I could use it to help set my expectations about what the implementation ought to be achieving. We don't always test against the human-oriented command output since of course it can be kinda brittle, but in this situation where the logic is embedded in the command implementation itself I think it can be pragmatic to do some substring testing as long as we're careful to make the test cases distinct enough to avoid false positives in the case of future regressions.

Alternatively, perhaps it would help to factor out that logic into a separate function that takes oldLock and platformLock and returns one of three values to direct the UI about which message it should show, and then we could test that helper function directly and not worry about testing the UI output because the logic in the command will presumably then just be a relatively simple switch statement, or similar. 🤔

These are all just some ideas. If you have a different direction in mind then that's fine too! Let me know if I seem to be missing the point entirely! 😀

[apparentlymart]

My understanding of this call and the function it's calling is that it will return true only if platformLock doesn't add any new hashes compared to oldLock, which feels opposite to the message it's printing. Am I misunderstanding the behavior of ContainsAll? 🤔

[liamcervante]

Good spot! I had my logic the wrong way, I've fixed it now. I followed the second idea, so split this out into a function and added some tests. Left a simple switch statement in the original function.

[apparentlymart]

Minor: this seems like a good opportunity to use t.Errorf to allow both of these to potentially fail together if the function really misbehaves, since these two checks seem independent.

[liamcervante]

Done!

[apparentlymart]

Great, thanks!

I left a little style nit inline that I'm sorry I didn't catch the first time (or maybe it wasn't there the first time? I'm not sure! 😀) but no need for another code review round-trip just for that.

[apparentlymart]

This doesn't look like what our usual tools would generate, but I must admit I'm not entirely sure what usual tool is responsible for it, and since the checks all passed I guess it might be something we have in our local editors only rather than something enforced by our CI process. 😖

Our typical basic style here is to separate the standard library packages from the external ones, with the standard library ones first and then a blank line followed by the external ones, with each separate "block" of packages in lexical order. I'd suggest manually changing this to match that for now, and then separately perhaps we can figure out which of the tools we're missing from our CI checks to catch this automatically in the future.

For some files that have a lot of imports we do also sometimes split the Terraform-internal packages from other external packages, so there would then be three "blocks" of packages: standard library, external, and then Terraform-internal. However, that's a more subjective call that our tooling does not enforce, and this one doesn't seem like it has enough packages to warrant that amount of fussiness, so I'm mentioning it only in the interests of helping you to see the patterns in other files in future. 😀

[jbardin]

The "default" grouping for imports is whatever goimports creates automatically, which is usually bundled with the standard group of editor plugins. The CI checks only verify what go fmt produces, which does not re-order imports on its own. It would be good to get that tool (or equivalent) working, since anyone else opening the file is going to rewrite it in their next commit.

[liamcervante]

Fixed! And yeah it looks like goimports isn't installed by Goland automatically: https://www.jetbrains.com/help/go/integration-with-go-tools.html#goimports

I have now configured it.

Brief sidebar into/shoutout for https://pre-commit.com/ as something I've used in the past to make sure CI/CD and pre-commit checks on engineers machines are consistent (works across IDEs/architectures, forces common versions for all tools, etc).

[github-actions]

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

[github-actions]

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.