create installer event for tracking provider lock hashes #31406
Conversation
There was a problem hiding this comment.
Great! I think this strikes a nicer compromise of being a bit more certain about the advice we're generating in this warning.
I have some little notes inline just about naming where things didn't resonate with my mental model while I was reading. Naming is subjective of course, so feel free to disagree with my suggestions!
| // It not just likely, but expected that there will be duplicates shared | ||
| // between all three collections of hashes i.e. the local hash and remote | ||
| // hashes could already be in the cached hashes. | ||
| ProvidersLockUpdated func(provider addrs.Provider, version getproviders.Version, local getproviders.Hash, remote []getproviders.Hash, cached []getproviders.Hash) |
There was a problem hiding this comment.
This seems like a reasonable design for this installer event indeed, though it did take me a little "squinting" to get my head around what each of these arguments means, so I have some suggestions for other names that I think would resonate better with me at least, based on my model of how the protocol behaves:
localHashes: hashes computed on the local system by analyzing files on disk. It is true that right now there can be only one, but the system was designed to support adding new hashing schemes in future and so I'd prefer to keep this plural just so the system design is consistent about that so it's clearer to a future maintainer how they might plug in a hypotheticalh2:scheme while retaining theh1:scheme. (YAGNI perhaps, but with other parts of this already designed around this possibility I'd like to keep things consistent so we'll have an easier time evolving this design in future.)signedHashes: hashes that were signed by the private key that the origin registry claims is the owner of this provider. (Note: "origin registry" is the term we use to refer to the registry whose hostname is embedded in the provider source address, so e.g.registry.terraform.io/hashicorp/awshasregistry.terraform.ioas its "origin registry".)priorHashes: hashes that were already present in the lock file before we made any changes. "prior" instead of "cached" I don't really have any strong argument for other than that "cached" makes me think of the provider plugin cache just due to the overlapping noun, and so "prior" hopefully avoids that false cognate.
There was a problem hiding this comment.
Perhaps we could document a guarantee here that these sets of hashes will always be presented in the same arbitrary-but-consistent order we use within the serialized lock file, so we can get the same ease-of-testing and ease-of-comparison benefits we get from there.
(This does seem to be asking for a "set of hashes" data type, rather than using a sorted slice by convention only, but I'm expecting the stdlib will get a "set of T" type before too long now that we have generic type support in the language so seems reasonable to hold on until we're ready to use the stdlib one, rather than writing yet another Terraform-specific set type -- of which we already have quite a few! 😬 )
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
…#31399) * terraform init: add warning and guidance when lock file is incomplete * make the provider list in the warning deterministic * create installer event for tracking provider lock hashes (#31406) * create installer event for tracking provider lock hashes * address comments * fix tests * improve error message * Update internal/command/init.go Co-authored-by: Martin Atkins <mart@degeneration.co.uk> Co-authored-by: Martin Atkins <mart@degeneration.co.uk>
|
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. |
This is an attempt to implement @apparentlymart's suggestion in #31399.
I can merge this back into the original PR if we like it.
I also noticed
HashPackageFailurewas not in use so removed it.