fix: Caching the same provider with different versions from various sources #3935
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes refactor the provider cache server initialization and request handling. Provider handlers are now initialized using a consolidated function that streamlines the logic for direct, proxy, filesystem, and network mirror installations. The codebase has been updated with new error types and improved logging and configuration, while obsolete mirror handler implementations have been removed. Controllers and server initialization now use explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CacheServer as ProviderCacheServer
participant NewHandlers as NewProviderHandlers
participant ProxyHandler as ProxyProviderHandler
participant ProviderService
participant ProviderController
Client->>CacheServer: Request cache server initialization
CacheServer->>NewHandlers: Call NewProviderHandlers(cliCfg, logger, registryNames)
NewHandlers-->>CacheServer: Return list of Provider Handlers
CacheServer->>ProxyHandler: Instantiate ProxyProviderHandler
CacheServer->>ProviderService: Set up ProviderService
CacheServer->>ProviderController: Configure with ProxyHandler & ProviderService
Client->>ProviderController: Request provider data (versions/platform)
ProviderController->>ProxyHandler: Proxy request for data
ProxyHandler-->>ProviderController: Return modified response
ProviderController-->>Client: Respond with provider information
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
levkohimins
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
tf/cache/helpers/client.go (2)
15-21: Nice use of concurrency-safe map for caching responses.Storing bytes in memory for all requests can grow large over time, though. You might want to implement an LRU or TTL to free up cache space and avoid hogging memory for an extended period.
23-29: Consider specifying timeouts on the underlying http.Client.Right now, the default http.Client can hang forever if the remote side never responds. Setting some sensible timeouts (e.g.,
Timeout,Transport.IdleConnTimeout) can prevent resource leaks.tf/cache/config.go (2)
61-66: Correct the “Porxy” spelling in the function name.Seems like a simple slip of the keyboard. Let’s fix it to “Proxy” to avoid confusion.
-func WithPorxyProviderHandler(handler *handlers.ProxyProviderHandler) Option { +func WithProxyProviderHandler(handler *handlers.ProxyProviderHandler) Option {
68-73: Double-check default usage of cacheProviderHTTPStatusCode.If this value defaults to 0 or is unset, you may return
0as the status code. If that’s not intended, consider a safe default (like423) to ensure a predictable workflow.tf/cache/handlers/network_mirror_provider.go (2)
29-33: Validate error messages for clarity
Love that you're returning early if URL parsing fails. But it might be worth clarifying the error message to make sure it’s obvious what failed, for smoother debugging experiences.-if err != nil { - return nil, errors.New(err) +if err != nil { + return nil, errors.WithMessage(err, "failed to parse network mirror URL in NewNetworkMirrorProviderHandler") }
72-99: Unused parameter: 'downloaderController'
downloaderControllerisn’t referenced inGetPlatform. If not intended for future use, removing it might simplify the method signature and reduce confusion.-func (handler *NetworkMirrorProviderHandler) GetPlatform(ctx context.Context, provider *models.Provider, downloaderController router.Controller) (*models.ResponseBody, error) { +func (handler *NetworkMirrorProviderHandler) GetPlatform(ctx context.Context, provider *models.Provider) (*models.ResponseBody, error) {tf/cache/handlers/direct_provider.go (1)
62-84: Provide clearer feedback if the platform is missing
If the requested platform doesn't exist, you returnnilwithout an explicit error. Some folks might appreciate a friendlier error message about the missing platform or an empty response for debugging.tf/cache/handlers/filesystem_mirror_provider.go (2)
78-81: Check for absolute paths on Windows
String-based checks like"://"versus local paths can get tricky on Windows with backslashes. You might want to systematically usefilepath.ToSlashor similar to avoid confusion.
76-86: Return an error if the platform is missing
Ifarchivedoesn't exist for the requested platform, the method quietly returnsnil. If that's not by design, consider an error or fallback strategy—especially if your usage relies on a guaranteed platform result.cli/provider_cache.go (2)
101-103: Handle errors more gracefully.
WhenNewProviderHandlersreturns an error, consider wrapping it to add more context, e.g."creating provider handlers failed". This can help debug issues in real environments.- return nil, err + return nil, fmt.Errorf("creating provider handlers failed: %w", err)
110-110: Rename to match the rest of the codebase.
WithProviderServiceis consistent, but double-check that all references align with the naming convention used across your code (likeAddProviderServiceorSetProviderServiceif that’s standard).tf/cache/handlers/provider.go (1)
34-70: Check naming and logging.
The functionNewProviderHandlersis well structured, but consider:
- Renaming
directIsdefinedtodirectIsDefinedfor clarity.- Logging or commenting when a direct method is automatically added, so users don’t wonder why it’s suddenly there.
- directIsdefined bool + directIsDefined bool ... if !directIsDefined { logger.Infof("No direct provider installation method defined; adding a default direct method...") providerHandlers = append(...) }tf/cache/handlers/proxy_provider.go (1)
38-47: Validate logger and credentials.
InNewProxyProviderHandler, it might help to verify logger and creds are non-nil or default them if they are. This keeps the handler from blowing up in corner cases.func NewProxyProviderHandler(logger log.Logger, credsSource *cliconfig.CredentialsSource) *ProxyProviderHandler { + if logger == nil { + // fallback or error out + } + if credsSource == nil { + // fallback or error out + } return &ProxyProviderHandler{ ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
cli/provider_cache.go(1 hunks)cli/provider_cache_test.go(2 hunks)pkg/log/format/format.go(1 hunks)pkg/log/format/placeholders/field.go(1 hunks)tf/cache/config.go(3 hunks)tf/cache/controllers/downloader.go(3 hunks)tf/cache/controllers/provider.go(4 hunks)tf/cache/handlers/common_provider.go(1 hunks)tf/cache/handlers/direct_provider.go(1 hunks)tf/cache/handlers/errors.go(1 hunks)tf/cache/handlers/filesystem_mirror_provider.go(1 hunks)tf/cache/handlers/network_mirror_provider.go(1 hunks)tf/cache/handlers/provider.go(3 hunks)tf/cache/handlers/provider_filesystem_mirror.go(0 hunks)tf/cache/handlers/provider_network_mirror.go(0 hunks)tf/cache/handlers/proxy_provider.go(6 hunks)tf/cache/handlers/registry_urls.go(4 hunks)tf/cache/helpers/client.go(1 hunks)tf/cache/helpers/http.go(4 hunks)tf/cache/helpers/reverse_proxy.go(3 hunks)tf/cache/middleware/logger.go(2 hunks)tf/cache/models/provider.go(2 hunks)tf/cache/server.go(2 hunks)tf/cache/services/provider_cache.go(1 hunks)
💤 Files with no reviewable changes (2)
- tf/cache/handlers/provider_network_mirror.go
- tf/cache/handlers/provider_filesystem_mirror.go
✅ Files skipped from review due to trivial changes (1)
- tf/cache/services/provider_cache.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (44)
tf/cache/handlers/errors.go (1)
3-9: Well-structured error type for URL not found scenariosNice addition of a custom error type for handling not-found URLs. The structure is clean and follows Go's idiomatic error handling patterns. Having a specific error type will make error handling more precise throughout the codebase.
pkg/log/format/placeholders/field.go (1)
13-16: Good addition of cache server log constantsThese new constants for cache server URL and status are well-documented and follow the existing naming convention. They'll make the logs more consistent and readable when dealing with provider caching operations.
pkg/log/format/format.go (1)
69-74: Log format enhanced with cache server fieldsThe addition of cache server URL and status fields to the log format is clean and consistent with the existing pattern. This will improve observability when debugging provider caching issues.
tf/cache/helpers/reverse_proxy.go (2)
22-22: Field visibility change improves modularityGood call changing
loggertoLogger. Making this field exported allows for better integration with other packages that need to access or modify the logger, which aligns with the PR's goal of improving provider caching from various sources.
59-59: Updated to use exported Logger fieldThis line properly updates the reference to use the newly exported
Loggerfield. Ensures logging functionality remains intact after the visibility change.tf/cache/controllers/downloader.go (4)
10-10: Good addition of the services package importThis import is needed for the new
ProviderServicefield that's being added to theDownloaderControllerstruct. Makes sense given the refactoring to use a dedicated service.
21-22: Nice refactoring of the controller dependenciesReplacing the
ProviderHandlersslice with specificProviderServiceandProxyProviderHandlerfields gives clearer intentions and better separation of concerns. This approach makes the code more maintainable by explicitly declaring dependencies rather than working with a generic slice of handlers.
50-54: Clean caching implementationThe new implementation is more straightforward and easier to follow. Instead of iterating through multiple handlers, you're directly using the
ProviderServiceto check for cached providers. The log message also provides helpful debugging information.
57-59: Simplified download logicUsing a dedicated
ProxyProviderHandlerfor downloads rather than iterating through a slice of handlers makes the code more direct and easier to understand. This change aligns well with the PR objective of "Caching the same provider but with different versions from various sources."tf/cache/helpers/http.go (4)
95-95: Consistent error handling - good changeWrapping the error with
errors.New(err)provides more consistent error handling throughout the codebase. This helps maintain a uniform error format that will make debugging easier.
113-113: Consistent error handling - good changeJust like the previous change, wrapping the error with
errors.New(err)improves error consistency.
133-133: Consistent error handling - good changeAnother good instance of using
errors.New(err)for consistent error handling.
146-146: Consistent error handling - good changeFinal instance of improving error handling consistency by using
errors.New(err).tf/cache/middleware/logger.go (2)
5-5: Good use of placeholders packageAdding this import allows the use of predefined key constants for logging, which is more maintainable than hardcoded strings.
16-22: Improved logging with placeholdersNice refactoring here! Using the placeholder constants instead of hardcoded strings makes the code more maintainable. I also like that you explicitly marked the unused context parameter with an underscore. The structured logging approach with field names will make debugging and log analysis much easier.
tf/cache/models/provider.go (6)
55-55: Good addition of the Versions typeCreating a clear type definition for
Versionsimproves code readability and makes it easier to add methods to this collection in the future if needed.
58-61: Clean refactoring of the Version structUsing the new
Platformstype for thePlatformsfield makes the code more consistent and clearer to understand.
63-65: Useful String() method additionAdding a
String()method to theVersionstruct makes debugging easier and improves logging output. The format including version, protocols, and platforms makes it easy to identify specific versions.
67-67: Good addition of the Platforms typeSimilar to the
Versionstype, creating a dedicated type forPlatformsimproves code clarity and makes it easier to add methods to this collection in the future.
74-76: Useful String() method for PlatformAdding a
String()method to thePlatformstruct improves logging and debugging capabilities. The format with OS and architecture is clear and concise.
143-147: Better String() implementation for ProviderGood improvement to conditionally include the version information only when it's available. This makes the string representation more accurate for incomplete Provider objects.
tf/cache/server.go (3)
33-36: Great improvement on explicitly injecting dependencies to DownloaderControllerThis change provides better clarity and maintainability by making the dependencies explicit rather than relying on the previous opaque
ProviderHandlersfield. This will make it easier to understand and track how the components interact.
38-46: Nice refactoring of ProviderController with explicit dependenciesThe controller now has clear and explicit dependencies which improves readability and maintainability. I particularly like how you've:
- Added dedicated fields for different handler types
- Included the logger directly instead of retrieving it elsewhere
- Made the HTTP status code configurable
This approach makes the dependencies much clearer and follows good dependency injection practices.
63-63: Simplified services initializationUsing a slice with just the provider service makes the code more direct and less error-prone than using the previous
cfg.servicesapproach, which was likely a more general collection of services.cli/provider_cache_test.go (1)
51-51: Good addition of HTTP status code optionThis reflects the changes in the main code and ensures the tests appropriately verify this functionality.
tf/cache/handlers/registry_urls.go (4)
27-34: Good addition of common connection errors for offline detectionThis comprehensive list of common connection errors will help the system properly identify offline situations, leading to better graceful degradation when connectivity is limited.
41-47: Improved String() method with error handlingThe updated implementation properly handles JSON marshaling errors and provides a fallback representation when marshaling fails. This makes the code more robust.
65-65: Updated error type for better error handlingUsing the structured
NotFoundWellKnownURLErrortype instead of a string error provides better context and enables more sophisticated error handling downstream.
84-97: Well-implemented offline error detection functionThis function nicely consolidates the logic for determining if an error relates to offline status. The dual approach of checking both by direct error comparison and string matching provides robust detection.
tf/cache/handlers/common_provider.go (4)
12-23: Good introduction of CommonProviderHandler structThe struct is well-designed with clear documentation. I particularly like the use of xsync.MapOf instead of sync.Map for better performance and type safety.
25-43: Clear and robust constructor implementationThe constructor properly handles optional inputs (nil includes/excludes) and initializes all necessary fields. The use of models.ParseProviders to handle pattern conversion is clean.
45-55: Well-structured provider handling logicThe logic flow is clear and follows a sensible order:
- First check exclusions (which take priority)
- Then check inclusions if any exist
- Default to accepting all providers if no inclusions are specified
This approach makes the handler's behavior predictable and easy to understand.
57-78: Excellent caching implementation for registry URLsThe DiscoveryURL method effectively:
- Checks the cache first to avoid redundant network calls
- Handles offline errors gracefully by falling back to default URLs
- Provides helpful debug logging
- Updates the cache for future requests
This will significantly improve performance and resilience when handling provider requests, especially in environments with limited connectivity.
tf/cache/helpers/client.go (2)
31-63: Clarify behavior for non-200 responses.Currently,
decodeResponsereturns(nil, nil)if the status isn’t 200, and the code caches that nil result. That means callers won’t see an error: they’ll just see an empty struct. Make sure that’s intended or handle non-200 codes more explicitly, so the consumer knows the request failed.
65-75: JSON Unmarshal flow looks great.The function is straightforward and uses standard JSON decoding. It's a nice clean design.
tf/cache/controllers/provider.go (2)
74-98: Return a specific status when no versions are found.This endpoint always responds with 200, even if no versions exist or handlers fail. You might consider returning 404 or a similar code if zero versions were found, so clients know it didn’t succeed in the usual sense.
121-148: Validate “nil” response scenario.If all handlers fail,
respremains nil but we still proceed to cache. Depending on your logic, you might need to propagate an error or at least signal that no valid platform data was returned.tf/cache/handlers/network_mirror_provider.go (1)
47-69: Consider how the 'availablePlatforms' property is populated
You're appending amodels.VersionusingavailablePlatforms, but the snippet doesn't show whereavailablePlatformsis declared or set. If it’s an uninitialized placeholder, you risk unexpected null references or empty data.tf/cache/handlers/direct_provider.go (2)
24-29: Constructor looks good
This straightforward constructor approach is clear and neat. Great job.
39-60: Double-check partial or empty responses
When callingDoto retrieve versions, consider how you handle partial or empty JSON responses. If the returnedversions.Versionsis nil or empty, you might want to log a warning or handle it gracefully.tf/cache/handlers/filesystem_mirror_provider.go (1)
94-95: Confirm desired behavior on missing files
readMirrorDatareturnsnilwhen the file doesn’t exist. Make sure that silently ignoring a non-existent JSON is intentional. If the data is essential, returning an error might be more direct.cli/provider_cache.go (1)
99-99: Great introduction of the proxy provider handler.
This new instantiation looks neat. Just ensure you’ve validated in at least one test that the proxy handler is wired correctly.
[approve]tf/cache/handlers/proxy_provider.go (2)
88-88: Neat approach for response modification.
Wrapping the reverse proxy response in a function that tweaks JSON is a clever solution. Great job making it modular.
120-120: Double-check path joining.
InmodifyDownloadURLsInJSONBody, combining the host and path can lead to double slashes if either ends in “/”. Consider normalizing or trimming slashes beforepath.Jointo avoid weird behavior.Also applies to: 142-142
| @@ -1,4 +1,4 @@ | |||
| package handlers | |||
| package helpers | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Package name change from "handlers" to "helpers"
The package name change seems appropriate given the utility nature of this code. Just make sure all imports throughout the codebase have been updated to reflect this change.
🏁 Script executed:
#!/bin/bash
# Check for any references to the old package path that might have been missed
rg "\"github.com/gruntwork-io/terragrunt/tf/cache/handlers\"" --type goLength of output: 653
Attention: Update Remaining Import Paths for Package Renaming
The change in tf/cache/helpers/reverse_proxy.go from package handlers to package helpers looks good for its utility purpose. However, our search revealed that several files still reference the old package path "github.com/gruntwork-io/terragrunt/tf/cache/handlers". Please update the import paths in the following files to reflect the new package location:
- cli/provider_cache_test.go
- cli/provider_cache.go
- tf/cache/server.go
- tf/cache/handlers/provider_test.go
- tf/cache/config.go
- tf/cache/controllers/provider.go
- tf/cache/controllers/downloader.go
Once these changes are made, the entire codebase will be consistent with the new helpers package naming. Thanks for handling this!
cli/provider_cache_test.go
Outdated
| logger := log.New() | ||
|
|
||
| providerService := services.NewProviderService(providerCacheDir, pluginCacheDir, nil, log.New()) | ||
| providerHandler := handlers.NewProviderDirectHandler(providerService, cli.CacheProviderHTTPStatusCode, new(cliconfig.ProviderInstallationDirect), nil) | ||
| providerService := services.NewProviderService(providerCacheDir, pluginCacheDir, nil, logger) | ||
| providerHandler := handlers.NewDirectProviderHandler(logger, new(cliconfig.ProviderInstallationDirect), nil) | ||
| proxyProviderHandler := handlers.NewProxyProviderHandler(logger, nil) | ||
|
|
||
| testCase.opts = append(testCase.opts, cache.WithServices(providerService), cache.WithProviderHandlers(providerHandler)) | ||
| testCase.opts = append(testCase.opts, | ||
| cache.WithProviderService(providerService), | ||
| cache.WithProviderHandlers(providerHandler), | ||
| cache.WithPorxyProviderHandler(proxyProviderHandler), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test setup refactored to match new initialization pattern, but watch for the typo
The test setup now properly aligns with the changes made in the main codebase, but there appears to be a typo in one of the method names.
testCase.opts = append(testCase.opts,
cache.WithProviderService(providerService),
cache.WithProviderHandlers(providerHandler),
- cache.WithPorxyProviderHandler(proxyProviderHandler),
+ cache.WithProxyProviderHandler(proxyProviderHandler),
)The method name has "Porxy" instead of "Proxy" which could cause errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger := log.New() | |
| providerService := services.NewProviderService(providerCacheDir, pluginCacheDir, nil, log.New()) | |
| providerHandler := handlers.NewProviderDirectHandler(providerService, cli.CacheProviderHTTPStatusCode, new(cliconfig.ProviderInstallationDirect), nil) | |
| providerService := services.NewProviderService(providerCacheDir, pluginCacheDir, nil, logger) | |
| providerHandler := handlers.NewDirectProviderHandler(logger, new(cliconfig.ProviderInstallationDirect), nil) | |
| proxyProviderHandler := handlers.NewProxyProviderHandler(logger, nil) | |
| testCase.opts = append(testCase.opts, cache.WithServices(providerService), cache.WithProviderHandlers(providerHandler)) | |
| testCase.opts = append(testCase.opts, | |
| cache.WithProviderService(providerService), | |
| cache.WithProviderHandlers(providerHandler), | |
| cache.WithPorxyProviderHandler(proxyProviderHandler), | |
| ) | |
| logger := log.New() | |
| providerService := services.NewProviderService(providerCacheDir, pluginCacheDir, nil, logger) | |
| providerHandler := handlers.NewDirectProviderHandler(logger, new(cliconfig.ProviderInstallationDirect), nil) | |
| proxyProviderHandler := handlers.NewProxyProviderHandler(logger, nil) | |
| testCase.opts = append(testCase.opts, | |
| cache.WithProviderService(providerService), | |
| cache.WithProviderHandlers(providerHandler), | |
| cache.WithProxyProviderHandler(proxyProviderHandler), | |
| ) |
| func decodeResponse(resp *http.Response) ([]byte, error) { | ||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, nil | ||
| } | ||
|
|
||
| buffer, err := ResponseBuffer(resp) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| bodyBytes, err := io.ReadAll(buffer) | ||
| if err != nil { | ||
| return nil, errors.New(err) | ||
| } | ||
|
|
||
| resp.Body = io.NopCloser(buffer) | ||
|
|
||
| return bodyBytes, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle non-200 status codes with an explicit error.
Returning (nil, nil) makes the caller assume success. Consider returning an error if resp.StatusCode != http.StatusOK so you don’t silently swallow server failures.
cli/provider_cache.go
Outdated
| cache.WithPorxyProviderHandler(proxyProviderHandler), | ||
| cache.WithCacheProviderHTTPStatusCode(CacheProviderHTTPStatusCode), |
There was a problem hiding this comment.
Fix what looks like a spelling mistake.
cache.WithPorxyProviderHandler is probably missing an ‘r’ in “Proxy.” This would cause a compilation error.
- cache.WithPorxyProviderHandler(proxyProviderHandler),
+ cache.WithProxyProviderHandler(proxyProviderHandler),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cache.WithPorxyProviderHandler(proxyProviderHandler), | |
| cache.WithCacheProviderHTTPStatusCode(CacheProviderHTTPStatusCode), | |
| cache.WithProxyProviderHandler(proxyProviderHandler), | |
| cache.WithCacheProviderHTTPStatusCode(CacheProviderHTTPStatusCode), |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tf/cache/handlers/network_mirror_provider.go (2)
45-68: The GetVersions implementation could use a small improvement.The method correctly fetches and processes versions from the network mirror, but it uses a global
availablePlatformsvariable (line 63) that isn't defined in this file.Consider either:
- Creating a local platforms list based on the actual available platforms in the mirror data
- Adding a comment explaining where the global variable is defined
70-98: GetPlatform implementation looks solid, with a minor improvement opportunity.The implementation correctly fetches platform-specific data and constructs the response, but it might benefit from adding a debug log when a platform isn't found in the mirror data.
Consider adding something like:
if archive, ok := mirrorData.Archives[provider.Platform()]; ok { resp = (&models.ResponseBody{ Filename: filepath.Base(archive.URL), DownloadURL: archive.URL, }).ResolveRelativeReferences(handler.networkMirrorURL.ResolveReference(&url.URL{ Path: path.Join(handler.networkMirrorURL.Path, provider.Address()), })) +} else { + handler.logger.Debugf("Platform %q not found in network mirror for provider %s", provider.Platform(), provider.Address()) }tf/cache/handlers/filesystem_mirror_provider.go (1)
36-57: Potential improvement for platform availabilityThe
GetVersionsmethod currently assumes all versions support all available platforms by settingavailablePlatformsfor every version. This might not be accurate if some versions only support a subset of platforms.Consider refining this to check which platforms are actually available for each version, perhaps by examining the directory structure or the version-specific JSON files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cli/provider_cache.go(1 hunks)cli/provider_cache_test.go(2 hunks)tf/cache/config.go(3 hunks)tf/cache/controllers/provider.go(4 hunks)tf/cache/handlers/direct_provider.go(1 hunks)tf/cache/handlers/filesystem_mirror_provider.go(1 hunks)tf/cache/handlers/network_mirror_provider.go(1 hunks)tf/cache/handlers/provider.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unessential
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (19)
cli/provider_cache_test.go (2)
51-51: Good addition of the status code configuration.The addition of
cache.WithCacheProviderHTTPStatusCodealigns well with the changes in the controller implementation which now uses this status code when returning cache provider responses.
115-125: The test setup now matches the new architecture. Nice work.The initialization pattern has been updated to follow the new architecture:
- Creating a dedicated logger instance
- Properly initializing the provider service with that logger
- Using the new direct provider handler constructor
- Adding the new proxy provider handler
This setup ensures the test properly exercises the refactored code, which should help catch any provider caching issues with different versions from various sources.
tf/cache/config.go (3)
47-52: Clear interface evolution from services array to specific service types.The refactoring from
WithServicestoWithProviderServicemakes the API more explicit and type-safe by accepting a specific provider service instead of a generic slice of services.
61-73: Good additions to support the provider caching enhancement.These new configuration options:
WithProxyProviderHandler- Allows configuring a proxy provider handlerWithCacheProviderHTTPStatusCode- Enables customizing the status codeThese options directly support the PR's goal of better handling providers with different versions from various sources.
88-92: Config struct updated to match the new architectural approach.The structure now properly reflects the more explicit architecture with dedicated fields for the provider service, handlers collection, proxy handler, and status code configuration.
tf/cache/controllers/provider.go (3)
25-34: Controller structure updated with explicit dependencies.The controller now has explicit dependencies for everything it needs:
- Logger for better diagnostics
- Provider handlers collection for gathering data
- Dedicated proxy handler for redirection
- HTTP status code for caching responses
This makes the controller's responsibilities and dependencies much clearer.
74-97: Improved version collection with better error handling.The new implementation properly:
- Collects versions from all available handlers
- Logs errors without failing completely
- Returns a well-structured JSON response
This is more resilient than before and will better handle providers with multiple versions from different sources.
120-147: Enhanced platform handling with proxy support.The updated code now:
- Delegates to the proxy handler when not in caching mode
- Properly logs errors during platform retrieval
- Explicitly starts caching and returns the configured status code
This implementation better supports the PR goal of handling providers from various sources.
tf/cache/handlers/network_mirror_provider.go (1)
19-43: Good implementation of the network mirror handler.The handler structure follows the established pattern and properly implements the
ProviderHandlerinterface. The constructor properly parses and validates the network mirror URL.tf/cache/handlers/direct_provider.go (4)
15-21: Clean interface implementation with clear structureThe DirectProviderHandler structure is well-designed with embedded CommonProviderHandler and a client field. Using the
var _ ProviderHandler = new(DirectProviderHandler)pattern is a great way to ensure compile-time interface compliance.
23-28: Good constructor patternThe constructor function follows good practices by accepting dependencies and initializing all fields. I particularly like how it reuses the CommonProviderHandler for shared functionality.
34-59: Well-implemented GetVersions method with good error handlingThe implementation correctly:
- Fetches discovery URLs
- Constructs the request URL
- Makes the request and unmarshals the response
- Returns appropriate errors when things go wrong
Nice work on the detailed comment with the API documentation link!
61-83: Good implementation of GetPlatform with relative reference resolutionThe method properly handles platform-specific provider information retrieval. I particularly like that you're resolving relative references in line 80, which helps ensure URLs are properly formatted regardless of how they're returned from the API.
cli/provider_cache.go (2)
99-104: Great refactoring that simplifies provider handler initializationUsing the new consolidated
handlers.NewProviderHandlersfunction significantly improves the code by:
- Reducing duplication
- Making the provider handler creation process more maintainable
- Better error handling with a descriptive message
This is a solid improvement over manually constructing each handler type.
106-115: More explicit cache server configurationThe updated server initialization is more explicit about what's being configured:
WithProviderServiceinstead of the genericWithServices- Adding
WithProxyProviderHandlerseparately- Explicitly setting the cache HTTP status code
This makes the code more self-documenting and easier to understand.
tf/cache/handlers/provider.go (2)
33-69: Well-implemented provider handler factory functionThe
NewProviderHandlersfunction centralizes the logic for creating different types of provider handlers based on configuration. I particularly like how it:
- Pre-allocates slices with capacity hints
- Handles different installation method types with a clean switch statement
- Ensures at least one direct provider handler is available as a fallback
- Properly propagates errors from network mirror handler creation
This consolidation makes the code much more maintainable.
86-95: Improved ProviderHandler interfaceThe interface changes represent good API design improvements:
- Using
context.Contextinstead of framework-specificecho.Contextmakes the code more portable- Returning concrete types (
models.Versions,*models.ResponseBody) rather than just errors makes the API more explicit- Removing the
Downloadmethod suggests better separation of concernsThese changes make the interface cleaner and more focused.
tf/cache/handlers/filesystem_mirror_provider.go (2)
59-88: Well-implemented GetPlatform with URL handlingNice job on checking whether the URL contains a scheme and constructing a full path when necessary. This handles both absolute URLs and relative paths elegantly.
90-107: Good error handling in readMirrorDataI like that this helper method:
- Gracefully handles missing files by returning nil
- Properly wraps and propagates errors
- Has a focused responsibility (read and unmarshal JSON data)
This makes the code more robust and easier to maintain.
levkohimins
changed the title
Description
Fixes #3732.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Refactor
Chore