Initial integration of the provider dependency pinning work #26524
Conversation
These are helper functions to give the installation UI some hints about whether the lock file has changed so that it can in turn give the user advice about it. The UI-layer callers of these will follow in a later commit.
There was a problem hiding this comment.
This is great and I'm really excited to get it out in the world.
All of my inline comments are very small thoughts, not related to the actual functionality, and nothing that should stop you from merging this.
I have one bit of workflowicious feedback (which still doesn't stop this from being merged). Since this is a new feature that (slightly) changes the terraform init behavior, could we consider an "information only" mode for the first release (either by default, or via a command-line option, printing warnings during init instead of errors? I'm certainly ok with not doing this, or at least waiting to see what kind of feedback we actually get, but it also might help save us some user frustration if they run into weird edge cases (or bugs, now that I think of it).
| return ` | ||
| Usage: terraform providers lock [options] [providers...] | ||
|
|
||
| Normally the dependency lock file (.terraform.lock.hcl) is updated |
There was a problem hiding this comment.
I think we should say right in this help text that the providers will be (temporarily) downloaded to a temporary location; that makes sense once I read it but isn't what I had expected, and "surprise downloads" (no matter how small) can be off putting to some organizations.
| ) | ||
|
|
||
| func TestLocksEqual(t *testing.T) { | ||
| boopProvider := addrs.NewDefaultProvider("boop") |
| // Use this, rather than crafting comparison options yourself, in case the | ||
| // comparison strategy needs to change in future due to implementation details | ||
| // of the ProviderLock type. | ||
| var ProviderLockComparer cmp.Option |
There was a problem hiding this comment.
I really like that you've put this comparer right in the main package and am going to keep this in mind for future code! 🏅
There was a problem hiding this comment.
I think I got this from reading some issues in the go-cmp repository. Seemed like a nice pattern to keep callers from depending on the implementation details. 😀
| origin registry: | ||
|
|
||
| ``` | ||
| terraform providers lock \ |
There was a problem hiding this comment.
What do you think about an explicit option to use the provider installation configuration? I'm worried about organizations with weird setups - you probably know who I'm thinking of - who would need to run this command for every single provider instead of being able to say "this one provider isn't in the registry but everything else is".
There was a problem hiding this comment.
I had originally conceived this command as a way to temporarily ignore the installation methods in the CLI config in order to generate checksums in spite of the mirrors, and so I had originally thought that such an option would be superfluous because it would just mimic what terraform init does by default, but on reflection I suppose the combination of that option and multiple -platform could be interesting for the case of "I want to pre-lock multiple platforms", if you are using an installation method configuration that only overrides one or two in-house providers.
In other words, I agree with you 😀 but I will hold on including that in this PR because I want to think a bit more about what to call this option.
This changes the approach used by the provider installer to remember between runs which selections it has previously made, using the lock file format implemented in internal/depsfile. This means that version constraints in the configuration are considered only for providers we've not seen before or when -upgrade mode is active.
This probably isn't the best UI we could do here, but it's a placeholder for now just to avoid making it seem like we're ignoring the lock file and checking for new versions anyway.
This command is intended to help support situations where Terraform is configured to use only local mirrors for provider installation and so the normal "terraform init" flow would not have direct access to the official package checksums published in the origin registry. The intended workflow here is to use this command only when adding a new provider or changing an existing provider's version in the configuration, to augment the lock file with all of the checksums required to verify the provider across a variety of different platforms. Once this command has recorded all of the official checksums, future runs of "terraform init" will verify that provider packages obtained from a local mirror match with those upstream checksums.
This includes both the main documentation about the lock file itself and changes to related documentation about Terraform commands that interact with the lock file. We will likely continue to update this first pass of documentation as we get feedback and questions during the prerelease period.
Previously we were just letting hclwrite do its default formatting behavior here. The current behavior there isn't ideal anyway -- it puts big data structures all on one line -- but even ignoring that our goal for this file format is to keep things in a highly-normalized shape so that diffs against the file are clear and easy to read. With that in mind, here we directly control how we write that value into the file, which means that later changes to hclwrite's list/set presentation won't affect it, regardless of what form they take.
apparentlymart
force-pushed
the
f-provider-dep-pinning
branch
from
a1a8fb5
to
269c15f
Compare
|
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
locked as resolved and limited conversation to collaborators
This follows on from some earlier work that introduced models for representing provider dependency "locks" and a file format for saving them to disk. This PR wires the new models and behaviors into the
terraform initcommand, making it use the new user-facing lock file.terraform.lock.hclin place of the old implementation-detail.terraform/plugin/selections.jsonfile.This new file is intended to be checked in to version control. Although it's primarily intended for automatic updates rather than manual updates, we've switched to using HCL syntax for it because humans will see changes to the lock file as part of pull requests and (subjectively) diffs of the HCL-based locks language seem easier to read than diffs of a JSON data structure.
I still have work to do here in testing more edge cases, polishing the user experience, and adding test coverage for some behaviors that apparently weren't previously tested. However, I've been on a merge conflict treadmill for the past week due to working on this concurrently with code cleanup and refactoring, so I'd like to merge this checkpoint and then continue working on the details in subsequent PRs.
Because of that, I'd primarily like to focus this review on whether the functionality here seems to do what it sets out to do, and de-emphasize broader feedback about exactly how this fits into the workflow, how the UI behaves, etc. Both forms of feedback are welcome, but I'd ask you to be clear about which category your feedback fits into so I can understand what is blocking merging this PR and what I can address in future PRs. Ultimately my goal is to merge this soon before it becomes merge-conflicted again, addressing as much feedback as possible in separate PRs.
I specifically intend to make follow-up PRs with additional text coverage for the new command added here and for various
terraform initprovider installation situations that currently lack test coverage, but I decided to separate that just to avoid the drag of continuing merge conflicts.I've included in this PR a first draft of the user-facing documentation for this primarily because I think that's the best way to show the overall behavior I intend this to have. I expect that documentation to see at least one more editing revision after this though, so that I can do it with my "technical writing brain" inserted, rather than with my "programming brain".