refactor: Separate concerns in mock provider sources logic, handle callbacks using t.Cleanup#38446
Merged
SarahFrench merged 4 commits intomainfrom Apr 27, 2026
Merged
Conversation
SarahFrench
added
the
no-changelog-needed
…eMeta & FakePackageMetaViaHTTP. Handle cleanup internally. Co-authored-by: Copilot <copilot@github.com>
…rver` Co-authored-by: Copilot <copilot@github.com>
SarahFrench
force-pushed
the
sarah/refactor-mock-provider-sources
branch
from
8ddc6e7 to
4aab9d9
Compare
SarahFrench
commented
Apr 27, 2026
| // The caller is responsible for calling the close callback to clean up the temporary file. | ||
| // The temporary file is only used to calculate checksums and isn't actually used to install the provider in the test. | ||
| func FakePackageMetaViaHTTP(provider addrs.Provider, version Version, protocols VersionList, target Platform, locationBaseUrl string, execFilename string) (PackageMeta, func(), error) { | ||
| func FakePackageMetaViaHTTP(t *testing.T, provider addrs.Provider, version Version, protocols VersionList, target Platform, locationBaseUrl string) (PackageMeta, error) { |
Member
Author
There was a problem hiding this comment.
I'd recommend going commit-by-commit in this PR, but figured I'd highlight that this is the central part of the changes.
3 tasks
austinvalle
pushed a commit
that referenced
this pull request
Apr 28, 2026
…llbacks using `t.Cleanup` (#38446) * refactor: Split temp file creation logic out of FakeInstallablePackageMeta & FakePackageMetaViaHTTP. Handle cleanup internally. * refactor: Use 'Must' helpers in `newMockProviderSourceUsingTestHttpServer` * refactor: Update tests using `newMockProviderSource` to not handle 'close' callback
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For a separate PR I've been working on implementing a test helper that returns an http server that acts as a network mirror. For that to work the server needs to be able to serve data from files that 'look like' provider archives and have associated checksums.
That logic already exists but it was inside of
getproviders.FakeInstallablePackageMeta. That function both created temporary files and returnedPackageMetadata. I've split the file creation out into a newCreateFakeFileWithChecksumForProviderfunction that in future I will reuse when implementing the mock network mirror.During this refactoring I realised that there a lot of callbacks returned to calling code for cleanup. I've updated existing helpers and made the new helper use the passed in testing.T argument to handle cleanup internally via
t.Cleanup. I don't believe there's a special reason why this isn't already used in provider source-related helpers; when these helpers were originally added in March 2020 (#24477) the t.Cleanup function had only just been introduced to Go the month before and I imagine hadn't been adopted in the codebase yet.Note to reviewer:
Please go commit-by-commit to understand the diffs! I've split things up.
That final commit dramatically increased the footprint of this PR 😅 - we could avoid this by reverting the last commit and using the approach in 5c6fe51 , returning a no-op callback and leaving calling code unchanged. If we chose that approach we'd need to avoid copy-paste of existing tests and instead have new tests ignore the returned no-op callback. In my opinion it's better to rip the bandaid off now.
Target Release
1.16.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry