[WIP] feat: Support version constraints for Terraform Registry modules #3926
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce several new error types in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GTV as "GetTargetVersion()"
participant HTTP as "HTTP Client"
participant JSON as "JSON Parser"
participant Filter as "getHighestVersion()"
Caller->>GTV: Call with module info and versionQuery
GTV->>HTTP: Build URL and perform HTTP request
HTTP-->>GTV: Return JSON response
GTV->>JSON: Parse JSON into Modules struct
GTV->>Filter: Filter available versions by constraint
Filter-->>GTV: Return highest version
GTV-->>Caller: Return highest version or error
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tf/getter.go (1)
200-208: Consider removing the unused logger parameter.In
getHighestVersion(logger log.Logger, ...), theloggerisn’t used, which might confuse future readers. You can either utilize the logger for debug logs or drop it to keep the function signature clean.-func getHighestVersion(logger log.Logger, availableVersions []*version.Version) string { +func getHighestVersion(availableVersions []*version.Version) string { if len(availableVersions) == 0 { return "" } sort.Sort(version.Collection(availableVersions)) return availableVersions[len(availableVersions)-1].String() }🧰 Tools
🪛 golangci-lint (1.62.2)
201-201:
getHighestVersion-loggeris unused(unparam)
205-205: expressions should not be cuddled with blocks
(wsl)
206-206: return statements should not be cuddled if block has more than two lines
(wsl)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tf/errors.go(1 hunks)tf/getter.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tf/getter.go
171-171: expressions should not be cuddled with blocks
(wsl)
224-224: Error return value of json.Unmarshal is not checked
(errcheck)
201-201: getHighestVersion - logger is unused
(unparam)
233-233: only one cuddle assignment allowed before range statement
(wsl)
219-219: assignments should only be cuddled with other assignments
(wsl)
220-220: only one cuddle assignment allowed before if statement
(wsl)
223-223: declarations should never be cuddled
(wsl)
230-230: only one cuddle assignment allowed before if statement
(wsl)
238-238: if statements should only be cuddled with assignments
(wsl)
248-248: return statements should not be cuddled if block has more than two lines
(wsl)
205-205: expressions should not be cuddled with blocks
(wsl)
206-206: return statements should not be cuddled if block has more than two lines
(wsl)
🔇 Additional comments (7)
tf/getter.go (3)
155-155: Ensure consistent error checks for version query.Everything looks correct here, but be mindful that you've specifically disallowed multiple version queries while expecting exactly one. This is fine if it matches your design. Just keep an eye out for future expansions if you ever need to handle more than one version.
162-166: Straightforward invocation of GetTargetVersion.This call seamlessly integrates with the rest of the flow. Good job making the code more modular by separating out version resolution into its own function.
167-167: Great use of structured logging.You’re using
Logger.Infofwith the URL, which is handy for debugging. No concerns here.Also applies to: 171-171
tf/errors.go (4)
44-49: Solid error reporting for module fetch failures.This addition clearly identifies when module versions can’t be retrieved, making troubleshooting easier.
53-57: Useful for indicating unsatisfied constraints.A crisp error message for constraints that don’t match any available version. Perfect for clarifying the mismatch.
63-71: Clear message for malformed version constraints.Good practice to have a dedicated error for incorrectly formatted constraints. Makes debugging faster.
72-80: Handles malformed version strings gracefully.Distinguishing a malformed version from other errors is a nice touch. This keeps user-facing messages precise.
tf/getter.go
Outdated
| func GetTargetVersion(ctx context.Context, logger log.Logger, registryDomain string, moduleRegistryBasePath string, modulePath string, versionQuery string) (string, error) { | ||
| // Retrieve the available versions for the module | ||
| moduleRegistryBasePath = strings.TrimSuffix(moduleRegistryBasePath, "/") | ||
| modulePath = strings.TrimSuffix(modulePath, "/") | ||
| modulePath = strings.TrimPrefix(modulePath, "/") | ||
| moduleVersionsPath := fmt.Sprintf("%s/%s/versions", moduleRegistryBasePath, modulePath) | ||
| moduleVersionsURL, err := url.Parse(moduleVersionsPath) | ||
| if err != nil { | ||
| return "", errors.New(err) | ||
| } | ||
| body, _, err := httpGETAndGetResponse(ctx, logger, *moduleVersionsURL) | ||
| if err != nil { | ||
| return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) | ||
| } | ||
| var responseJSON Modules | ||
| json.Unmarshal(body, &responseJSON) | ||
| availableVersions := responseJSON.Modules[0].ModuleVersions | ||
|
|
||
| // Filter the available versions based on the version constraint | ||
| filteredVersions := []*version.Version{} | ||
| versionConstraint, err := version.NewConstraint(versionQuery) | ||
| if err != nil { | ||
| return "", errors.New(ModuleVersionConstraintMalformedErr{versionConstraint: versionQuery}) | ||
| } | ||
| for _, availableVersion := range availableVersions { | ||
| availableVersionParsed, err := version.NewVersion(availableVersion.Version) | ||
| if err != nil { | ||
| return "", errors.New(ModuleVersionMalformedErr{version: availableVersion.Version}) | ||
| } | ||
| if versionConstraint.Check(availableVersionParsed) { | ||
| filteredVersions = append(filteredVersions, availableVersionParsed) | ||
| } | ||
| } | ||
|
|
||
| // Get the highest version from the filtered versions | ||
| targetVersion := getHighestVersion(logger, filteredVersions) | ||
| if targetVersion == "" { | ||
| return "", errors.New(ModuleVersionConstraintErr{versionConstraint: targetVersion}) | ||
| } | ||
| return targetVersion, nil | ||
| } |
There was a problem hiding this comment.
Add error handling for json.Unmarshal and clarify the final constraint error.
- Unmarshal error check: Currently, the return value of
json.Unmarshal(body, &responseJSON)isn’t checked (line 224). This can mask malformed JSON scenarios. - Constraint error detail: In line 246, you’re returning
errors.New(ModuleVersionConstraintErr{versionConstraint: targetVersion})whentargetVersionis empty. This might be more accurate if you usedversionQueryso that the reported unsatisfied constraint matches the user input.
Apply this diff to add error handling and reference the original constraint in the error:
--- a/tf/getter.go
+++ b/tf/getter.go
@@ -223,7 +223,13 @@ func GetTargetVersion(ctx context.Context, logger log.Logger, registryDomain stri
var responseJSON Modules
- json.Unmarshal(body, &responseJSON)
+ if err := json.Unmarshal(body, &responseJSON); err != nil {
+ return "", errors.New(errors.New(
+ fmt.Sprintf("Failed to parse response JSON for module %s: %v",
+ modulePath, err)))
+ }
availableVersions := responseJSON.Modules[0].ModuleVersions
@@ -243,7 +249,7 @@ func GetTargetVersion(ctx context.Context, logger log.Logger, registryDomain stri
targetVersion := getHighestVersion(logger, filteredVersions)
if targetVersion == "" {
- return "", errors.New(ModuleVersionConstraintErr{versionConstraint: targetVersion})
+ return "", errors.New(ModuleVersionConstraintErr{versionConstraint: versionQuery})
}
return targetVersion, nilCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
224-224: Error return value of json.Unmarshal is not checked
(errcheck)
233-233: only one cuddle assignment allowed before range statement
(wsl)
219-219: assignments should only be cuddled with other assignments
(wsl)
220-220: only one cuddle assignment allowed before if statement
(wsl)
223-223: declarations should never be cuddled
(wsl)
230-230: only one cuddle assignment allowed before if statement
(wsl)
238-238: if statements should only be cuddled with assignments
(wsl)
248-248: return statements should not be cuddled if block has more than two lines
(wsl)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tf/getter.go (3)
230-243: Consider using early returns to improve readability.The version filtering logic can be simplified using early returns:
filteredVersions := []*version.Version{} versionConstraint, err := version.NewConstraint(versionQuery) if err != nil { return "", errors.New(ModuleVersionConstraintMalformedErr{versionConstraint: versionQuery}) } for _, availableVersion := range availableVersions { availableVersionParsed, err := version.NewVersion(availableVersion.Version) if err != nil { - return "", errors.New(ModuleVersionMalformedErr{version: availableVersion.Version}) + // Skip invalid versions instead of failing + continue } if versionConstraint.Check(availableVersionParsed) { filteredVersions = append(filteredVersions, availableVersionParsed) } }🧰 Tools
🪛 golangci-lint (1.62.2)
235-235: only one cuddle assignment allowed before range statement
(wsl)
232-232: only one cuddle assignment allowed before if statement
(wsl)
240-240: if statements should only be cuddled with assignments
(wsl)
200-207: Enhance function documentation.Let's add more context about the sorting behavior:
-// getHighestVersion returns the highest version from the list of versions, or an empty string if the list is empty. +// getHighestVersion returns the highest semantic version from the list of versions using natural version sorting +// (e.g., 2.0.0 is higher than 1.9.9). Returns an empty string if the list is empty. func getHighestVersion(availableVersions []*version.Version) string {🧰 Tools
🪛 golangci-lint (1.62.2)
205-205: expressions should not be cuddled with blocks
(wsl)
206-206: return statements should not be cuddled if block has more than two lines
(wsl)
171-171: Consider adding more detailed logging.The current log only shows the download URL. Consider adding version information for better traceability:
-tfrGetter.TerragruntOptions.Logger.Infof("Downloading module from %s", moduleURL.String()) +tfrGetter.TerragruntOptions.Logger.Infof("Downloading module from %s (version constraint: %s, resolved version: %s)", moduleURL.String(), versionQuery, targetVersion)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tf/getter.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tf/getter.go
224-224: only one cuddle assignment allowed before if statement
(wsl)
235-235: only one cuddle assignment allowed before range statement
(wsl)
250-250: return statements should not be cuddled if block has more than two lines
(wsl)
205-205: expressions should not be cuddled with blocks
(wsl)
206-206: return statements should not be cuddled if block has more than two lines
(wsl)
219-219: assignments should only be cuddled with other assignments
(wsl)
223-223: declarations should never be cuddled
(wsl)
227-227: assignments should only be cuddled with other assignments
(wsl)
232-232: only one cuddle assignment allowed before if statement
(wsl)
240-240: if statements should only be cuddled with assignments
(wsl)
🔇 Additional comments (1)
tf/getter.go (1)
45-58: Clean and well-structured data models!The new structs for handling registry responses are well-documented and follow Go best practices for JSON deserialization.
tf/getter.go
Outdated
| func GetTargetVersion(ctx context.Context, logger log.Logger, registryDomain string, moduleRegistryBasePath string, modulePath string, versionQuery string) (string, error) { | ||
| // Retrieve the available versions for the module | ||
| moduleRegistryBasePath = strings.TrimSuffix(moduleRegistryBasePath, "/") | ||
| modulePath = strings.TrimSuffix(modulePath, "/") | ||
| modulePath = strings.TrimPrefix(modulePath, "/") | ||
| moduleVersionsPath := fmt.Sprintf("%s/%s/versions", moduleRegistryBasePath, modulePath) | ||
| moduleVersionsURL, err := url.Parse(moduleVersionsPath) | ||
| if err != nil { | ||
| return "", errors.New(err) | ||
| } | ||
| bodyData, _, err := httpGETAndGetResponse(ctx, logger, *moduleVersionsURL) | ||
| if err != nil { | ||
| return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) | ||
| } | ||
| var responseJSON Modules | ||
| if err := json.Unmarshal(bodyData, &responseJSON); err != nil { | ||
| return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) | ||
| } | ||
| availableVersions := responseJSON.Modules[0].ModuleVersions | ||
|
|
||
| // Filter the available versions based on the version constraint | ||
| filteredVersions := []*version.Version{} | ||
| versionConstraint, err := version.NewConstraint(versionQuery) | ||
| if err != nil { | ||
| return "", errors.New(ModuleVersionConstraintMalformedErr{versionConstraint: versionQuery}) | ||
| } | ||
| for _, availableVersion := range availableVersions { | ||
| availableVersionParsed, err := version.NewVersion(availableVersion.Version) | ||
| if err != nil { | ||
| return "", errors.New(ModuleVersionMalformedErr{version: availableVersion.Version}) | ||
| } | ||
| if versionConstraint.Check(availableVersionParsed) { | ||
| filteredVersions = append(filteredVersions, availableVersionParsed) | ||
| } | ||
| } | ||
|
|
||
| // Get the highest version from the filtered versions | ||
| targetVersion := getHighestVersion(filteredVersions) | ||
| if targetVersion == "" { | ||
| return "", errors.New(ModuleVersionConstraintErr{versionConstraint: versionQuery}) | ||
| } | ||
| return targetVersion, nil | ||
| } |
There was a problem hiding this comment.
Add validation for empty response handling.
The function could panic if the registry returns an empty modules list. Let's add some guards:
var responseJSON Modules
if err := json.Unmarshal(bodyData, &responseJSON); err != nil {
return "", errors.New(ModuleVersionsErr{moduleName: modulePath})
}
+if len(responseJSON.Modules) == 0 {
+ return "", errors.New(ModuleVersionsErr{moduleName: modulePath})
+}
availableVersions := responseJSON.Modules[0].ModuleVersions📝 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.
| func GetTargetVersion(ctx context.Context, logger log.Logger, registryDomain string, moduleRegistryBasePath string, modulePath string, versionQuery string) (string, error) { | |
| // Retrieve the available versions for the module | |
| moduleRegistryBasePath = strings.TrimSuffix(moduleRegistryBasePath, "/") | |
| modulePath = strings.TrimSuffix(modulePath, "/") | |
| modulePath = strings.TrimPrefix(modulePath, "/") | |
| moduleVersionsPath := fmt.Sprintf("%s/%s/versions", moduleRegistryBasePath, modulePath) | |
| moduleVersionsURL, err := url.Parse(moduleVersionsPath) | |
| if err != nil { | |
| return "", errors.New(err) | |
| } | |
| bodyData, _, err := httpGETAndGetResponse(ctx, logger, *moduleVersionsURL) | |
| if err != nil { | |
| return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) | |
| } | |
| var responseJSON Modules | |
| if err := json.Unmarshal(bodyData, &responseJSON); err != nil { | |
| return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) | |
| } | |
| availableVersions := responseJSON.Modules[0].ModuleVersions | |
| // Filter the available versions based on the version constraint | |
| filteredVersions := []*version.Version{} | |
| versionConstraint, err := version.NewConstraint(versionQuery) | |
| if err != nil { | |
| return "", errors.New(ModuleVersionConstraintMalformedErr{versionConstraint: versionQuery}) | |
| } | |
| for _, availableVersion := range availableVersions { | |
| availableVersionParsed, err := version.NewVersion(availableVersion.Version) | |
| if err != nil { | |
| return "", errors.New(ModuleVersionMalformedErr{version: availableVersion.Version}) | |
| } | |
| if versionConstraint.Check(availableVersionParsed) { | |
| filteredVersions = append(filteredVersions, availableVersionParsed) | |
| } | |
| } | |
| // Get the highest version from the filtered versions | |
| targetVersion := getHighestVersion(filteredVersions) | |
| if targetVersion == "" { | |
| return "", errors.New(ModuleVersionConstraintErr{versionConstraint: versionQuery}) | |
| } | |
| return targetVersion, nil | |
| } | |
| func GetTargetVersion(ctx context.Context, logger log.Logger, registryDomain string, moduleRegistryBasePath string, modulePath string, versionQuery string) (string, error) { | |
| // Retrieve the available versions for the module | |
| moduleRegistryBasePath = strings.TrimSuffix(moduleRegistryBasePath, "/") | |
| modulePath = strings.TrimSuffix(modulePath, "/") | |
| modulePath = strings.TrimPrefix(modulePath, "/") | |
| moduleVersionsPath := fmt.Sprintf("%s/%s/versions", moduleRegistryBasePath, modulePath) | |
| moduleVersionsURL, err := url.Parse(moduleVersionsPath) | |
| if err != nil { | |
| return "", errors.New(err) | |
| } | |
| bodyData, _, err := httpGETAndGetResponse(ctx, logger, *moduleVersionsURL) | |
| if err != nil { | |
| return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) | |
| } | |
| var responseJSON Modules | |
| if err := json.Unmarshal(bodyData, &responseJSON); err != nil { | |
| return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) | |
| } | |
| if len(responseJSON.Modules) == 0 { | |
| return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) | |
| } | |
| availableVersions := responseJSON.Modules[0].ModuleVersions | |
| // Filter the available versions based on the version constraint | |
| filteredVersions := []*version.Version{} | |
| versionConstraint, err := version.NewConstraint(versionQuery) | |
| if err != nil { | |
| return "", errors.New(ModuleVersionConstraintMalformedErr{versionConstraint: versionQuery}) | |
| } | |
| for _, availableVersion := range availableVersions { | |
| availableVersionParsed, err := version.NewVersion(availableVersion.Version) | |
| if err != nil { | |
| return "", errors.New(ModuleVersionMalformedErr{version: availableVersion.Version}) | |
| } | |
| if versionConstraint.Check(availableVersionParsed) { | |
| filteredVersions = append(filteredVersions, availableVersionParsed) | |
| } | |
| } | |
| // Get the highest version from the filtered versions | |
| targetVersion := getHighestVersion(filteredVersions) | |
| if targetVersion == "" { | |
| return "", errors.New(ModuleVersionConstraintErr{versionConstraint: versionQuery}) | |
| } | |
| return targetVersion, nil | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
224-224: only one cuddle assignment allowed before if statement
(wsl)
235-235: only one cuddle assignment allowed before range statement
(wsl)
250-250: return statements should not be cuddled if block has more than two lines
(wsl)
219-219: assignments should only be cuddled with other assignments
(wsl)
223-223: declarations should never be cuddled
(wsl)
227-227: assignments should only be cuddled with other assignments
(wsl)
232-232: only one cuddle assignment allowed before if statement
(wsl)
240-240: if statements should only be cuddled with assignments
(wsl)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tf/getter.go (2)
209-254: Consider enhancing error messages for better debugging.The error handling is solid, but we could make the error messages more helpful:
if err := json.Unmarshal(bodyData, &responseJSON); err != nil { - return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) + return "", errors.New(ModuleVersionsErr{ + moduleName: modulePath, + details: fmt.Sprintf("failed to parse JSON response: %v", err), + }) }🧰 Tools
🪛 golangci-lint (1.62.2)
216-216: only one cuddle assignment allowed before if statement
(wsl)
227-227: if statements should only be cuddled with assignments
(wsl)
243-243: if statements should only be cuddled with assignments
(wsl)
238-238: only one cuddle assignment allowed before range statement
(wsl)
223-223: declarations should never be cuddled
(wsl)
224-224: only one cuddle assignment allowed before if statement
(wsl)
235-235: only one cuddle assignment allowed before if statement
(wsl)
167-172: Consider standardizing the log message format.The logging addition is helpful, but let's make it consistent with other log messages in the codebase. Consider using a more structured format:
-tfrGetter.TerragruntOptions.Logger.Infof("Downloading module from %s", moduleURL.String()) +tfrGetter.TerragruntOptions.Logger.Infof("Downloading Terraform module from registry: %s", moduleURL.String())🧰 Tools
🪛 golangci-lint (1.62.2)
171-171: expressions should not be cuddled with blocks
(wsl)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/_docs/04_reference/04-config-blocks-and-attributes.md(1 hunks)tf/getter.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tf/getter.go
171-171: expressions should not be cuddled with blocks
(wsl)
206-206: return statements should not be cuddled if block has more than two lines
(wsl)
216-216: only one cuddle assignment allowed before if statement
(wsl)
227-227: if statements should only be cuddled with assignments
(wsl)
243-243: if statements should only be cuddled with assignments
(wsl)
238-238: only one cuddle assignment allowed before range statement
(wsl)
223-223: declarations should never be cuddled
(wsl)
224-224: only one cuddle assignment allowed before if statement
(wsl)
205-205: expressions should not be cuddled with blocks
(wsl)
235-235: only one cuddle assignment allowed before if statement
(wsl)
🔇 Additional comments (3)
docs/_docs/04_reference/04-config-blocks-and-attributes.md (1)
99-100: Great documentation addition!The explanation of version constraints is clear and the reference to official Terraform documentation is helpful.
tf/getter.go (2)
45-58: Clean struct definitions!The structs are well-documented and follow Go conventions for JSON handling. The nested structure matches the expected JSON response format from the registry.
201-207: Nice implementation of version selection!The function handles edge cases well and makes good use of the version.Collection sort interface.
🧰 Tools
🪛 golangci-lint (1.62.2)
206-206: return statements should not be cuddled if block has more than two lines
(wsl)
205-205: expressions should not be cuddled with blocks
(wsl)
juan-vg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/integration_registry_test.go (1)
61-69: Consider enhancing this test to verify the resolved versionThe test successfully verifies that initialization works with version constraints, but doesn't validate that the correct version was actually selected.
Consider expanding the test to verify the specific version that was selected. Example approach:
func TestTerraformRegistryVersionResolution(t *testing.T) { t.Parallel() versionFixture := util.JoinPath(registryFixturePath, registryFixtureVersion) helpers.CleanupTerragruntFolder(t, versionFixture) - _, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt init --working-dir "+versionFixture) + stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt init --working-dir "+versionFixture) require.NoError(t, err) + + // Verify that the output contains a version that matches our constraints + versionPattern := regexp.MustCompile(`Installing terraform-aws-modules/iam/aws v(5\.[0-4][0-9]\.[0-9]+|5\.5[0-9]\.[0-9]+)`) + assert.True(t, versionPattern.MatchString(stdout), "Expected output to show installation of a version matching the constraints") }tf/getter.go (4)
200-207: Consider adding a test for empty version list handlingThe
getHighestVersionfunction correctly handles the case of an empty version list, but the current tests don't specifically test this edge case.Add a test case like:
func TestGetHighestVersionEmptyList(t *testing.T) { result := getHighestVersion([]*version.Version{}) assert.Equal(t, "", result, "Expected empty string for empty version list") }🧰 Tools
🪛 golangci-lint (1.62.2)
206-206: return statements should not be cuddled if block has more than two lines
(wsl)
205-205: expressions should not be cuddled with blocks
(wsl)
212-262: Improve documentation for better maintainabilityThe function implementation is solid, but could benefit from more detailed documentation.
Consider enhancing the function documentation with:
- Example usage
- Expected error types that callers should handle
- More context about the expected module path format
// GetTargetVersion retrieves the target version of the module based on the version constraint provided. This function // will return the highest version that satisfies the version constraint. If no version satisfies the constraint, an // error will be returned. +// +// Example: +// +// version, err := GetTargetVersion(ctx, logger, "registry.terraform.io", "/v1/modules/", +// "/terraform-aws-modules/vpc/aws", "~> 3.0") +// +// The function will return errors in these cases: +// - ModuleVersionsErr: If the module or its versions cannot be retrieved +// - ModuleVersionConstraintMalformedErr: If the version constraint syntax is invalid +// - ModuleVersionMalformedErr: If a version in the registry response is invalid +// - ModuleVersionConstraintErr: If no version satisfies the constraint🧰 Tools
🪛 golangci-lint (1.62.2)
231-231: declarations should never be cuddled
(wsl)
246-246: only one cuddle assignment allowed before range statement
(wsl)
232-232: only one cuddle assignment allowed before if statement
(wsl)
261-261: return statements should not be cuddled if block has more than two lines
(wsl)
235-235: if statements should only be cuddled with assignments
(wsl)
238-238: assignments should only be cuddled with other assignments
(wsl)
251-251: if statements should only be cuddled with assignments
(wsl)
235-237: Add more specific error messages for missing modulesThe current error doesn't distinguish between "module not found" and "module has no versions".
Consider more specific error messages:
if len(responseJSON.Modules) == 0 || len(responseJSON.Modules[0].ModuleVersions) == 0 { - return "", errors.New(ModuleVersionsErr{moduleName: modulePath}) + if len(responseJSON.Modules) == 0 { + return "", errors.New(ModuleVersionsErr{moduleName: modulePath, details: "module not found"}) + } + return "", errors.New(ModuleVersionsErr{moduleName: modulePath, details: "module has no published versions"}) }🧰 Tools
🪛 golangci-lint (1.62.2)
235-235: if statements should only be cuddled with assignments
(wsl)
257-260: Consider logging the selected version for traceabilityAdding logging here would help users understand which version was selected based on their constraints.
targetVersion := getHighestVersion(filteredVersions) if targetVersion == "" { return "", errors.New(ModuleVersionConstraintErr{versionConstraint: versionQuery}) } + logger.Infof("Selected version %s for module %s based on constraint %s", + targetVersion, modulePath, versionQuery) return targetVersion, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/fixtures/tfr/version/terragrunt.hcl(1 hunks)test/integration_registry_test.go(2 hunks)tf/getter.go(4 hunks)tf/getter_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tf/getter.go
171-171: expressions should not be cuddled with blocks
(wsl)
206-206: return statements should not be cuddled if block has more than two lines
(wsl)
231-231: declarations should never be cuddled
(wsl)
246-246: only one cuddle assignment allowed before range statement
(wsl)
232-232: only one cuddle assignment allowed before if statement
(wsl)
261-261: return statements should not be cuddled if block has more than two lines
(wsl)
235-235: if statements should only be cuddled with assignments
(wsl)
238-238: assignments should only be cuddled with other assignments
(wsl)
251-251: if statements should only be cuddled with assignments
(wsl)
205-205: expressions should not be cuddled with blocks
(wsl)
🔇 Additional comments (5)
test/fixtures/tfr/version/terragrunt.hcl (1)
1-8: Looks well-structured for testing version constraints!This test fixture configures a typical Terraform registry module with complex version constraints. The pattern
~>5.0, <5.51.0, !=5.50.0demonstrates multiple constraint types:
- Pessimistic version operator (
~>5.0) - any 5.x version- Upper bound (
<5.51.0) - versions below 5.51.0- Exclusion (
!=5.50.0) - specifically avoiding 5.50.0This will be valuable for testing the version resolution logic.
test/integration_registry_test.go (1)
20-20: Good addition of the version fixture constant.The constant naming is consistent with the existing fixture path constants.
tf/getter_test.go (1)
158-233: Great comprehensive test coverage for the version resolver!This table-driven test thoroughly covers the various version constraint scenarios:
- Fixed versions
- Pessimistic versioning (
~>)- Complex constraints with multiple conditions
- Error cases (invalid constraints, non-existent modules, etc.)
The tests provide a clear contract for how the version resolver should behave in different situations, which will make maintenance easier.
tf/getter.go (2)
46-58: Well-structured JSON mapping typesThese struct definitions provide a clean mapping for the registry API response. The naming is clear and follows Go conventions.
162-172: Good improvement to use version resolution and add loggingReplacing the direct version query with the
GetTargetVersionfunction improves the module handling. The added logging will also make debugging easier by showing which URL is being used for downloads.🧰 Tools
🪛 golangci-lint (1.62.2)
171-171: expressions should not be cuddled with blocks
(wsl)
juan-vg
changed the title
There was a problem hiding this comment.
Our linter found some issues in CI:
tf/getter.go:251:3: if statements should only be cuddled with assignments (wsl)
if versionConstraint.Check(availableVersionParsed) {
^
tf/getter.go:231:2: declarations should never be cuddled (wsl)
var responseJSON Modules
^
tf/getter.go:235:2: if statements should only be cuddled with assignments (wsl)
if len(responseJSON.Modules) == 0 || len(responseJSON.Modules[0].ModuleVersions) == 0 {
^
tf/getter.go:243:2: only one cuddle assignment allowed before if statement (wsl)
if err != nil {
^
tf/getter.go:206:2: return statements should not be cuddled if block has more than two lines (wsl)
return availableVersions[len(availableVersions)-1].String()
^
tf/getter.go:227:2: assignments should only be cuddled with other assignments (wsl)
bodyData, _, err := httpGETAndGetResponse(ctx, logger, *moduleVersionsURL)
^
tf/getter.go:232:2: only one cuddle assignment allowed before if statement (wsl)
if err := json.Unmarshal(bodyData, &responseJSON); err != nil {
^
tf/getter.go:171:2: expressions should not be cuddled with blocks (wsl)
tfrGetter.TerragruntOptions.Logger.Infof("Downloading module from %s", moduleURL.String())
^
tf/getter.go:246:2: only one cuddle assignment allowed before range statement (wsl)
for _, availableVersion := range availableVersions {
^
tf/getter.go:261:2: return statements should not be cuddled if block has more than two lines (wsl)
return targetVersion, nil
^
tf/getter.go:238:2: assignments should only be cuddled with other assignments (wsl)
availableVersions := responseJSON.Modules[0].ModuleVersions
^
tf/getter.go:205:2: expressions should not be cuddled with blocks (wsl)
sort.Sort(version.Collection(availableVersions))
^
tf/getter.go:224:2: only one cuddle assignment allowed before if statement (wsl)
if err != nil {
^
tf/getter.go:228:2: only one cuddle assignment allowed before if statement (wsl)
if err != nil {
^
INFO File cache stats: 403 entries of total size 2.3MiB
INFO Memory: 2474 samples, avg is 2695.3MB, max is 3747.3MB
INFO Execution took 4m11.814409778s
make: *** [Makefile:45: run-lint] Error 1
Exited with code exit status 2
Please request re-review when you've addressed these issues.
juan-vg
changed the title
|
Shame on me I forgot to run the linter... I'm on it! |
juan-vg
changed the title
|
Thanks for addressing those, @juan-vg . Upon re-review, I realized that the implementation you've added uses a query string parameter for the source URL: locals {
base_source = "tfr:///terraform-aws-modules/iam/aws"
version = "~>5.0, <5.51.0, !=5.50.0"
}
terraform {
source = "${local.base_source}?version=${local.version}"
}Instead of a new terraform {
source = "tfr:///terraform-aws-modules/iam/aws"
version = "~>5.0, <5.51.0, !=5.50.0"
}I believe a new |
| @@ -55,5 +56,14 @@ func testTerraformRegistryFetching(t *testing.T, modPath, expectedOutputKey stri | |||
| require.NoError(t, json.Unmarshal(stdout.Bytes(), &outputs)) | |||
| _, hasOutput := outputs[expectedOutputKey] | |||
| assert.True(t, hasOutput) | |||
| } | |||
|
|
|||
| func TestTerraformRegistryVersionResolution(t *testing.T) { | |||
There was a problem hiding this comment.
I think we need a test for the error case too, current tests only cover happy path and won't catch regression in error handling.
There was a problem hiding this comment.
Sure, I'm completely open to it. Do you have any idea or clarification about the path I should follow? In fact I tried to make the test more accurate by reading the stdout and trying to read the info log-line where the version picked was shown (to check the picked version), but I discovered logs are at a different level and this was not possible. Any help here is very welcome!
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| tfrGetter.TerragruntOptions.Logger.Infof("Downloading module from %s", moduleURL.String()) |
There was a problem hiding this comment.
I'm not entirely sure we need this log message at the Info level. It may produce multiple unused lines, so would it make more sense to set it to Debug instead?
There was a problem hiding this comment.
I have doubts also. However, since there's another Info log related to this and it includes the query string, I considered useful to have now this second log line clarifying which version is finally being downloaded without having to go some levels up (way far from getter.go) to change that behaviour.
As an example:
That said, I believe this is an intermediate step. The next one should be to implement the version attribute and properly handle the version constraints at an upper level (adding the ability to invalidate tg cache when there's a new version even if the code hasn't changed, among other changes). Then, both log lines should be addressed and merged into an more accurate one. WDYT?
There was a problem hiding this comment.
FYI: Since the scope of the PR has been changed to also support a new version attribute, and this will imply modifying way more than the tf getter, I believe this logs issue could be addressed also at the root, so this log line in getter won't make sense anymore
| // GetTargetVersion retrieves the target version of the module based on the version constraint provided. This function | ||
| // will return the highest version that satisfies the version constraint. If no version satisfies the constraint, an | ||
| // error will be returned. | ||
| func GetTargetVersion(ctx context.Context, logger log.Logger, url url.URL, versionQuery string) (string, error) { |
There was a problem hiding this comment.
It appears that this function is only used within the tf package. Would it make sense to mark it as private?
There was a problem hiding this comment.
I tried to make it private, but then I get an error on the getter_test :/
There was a problem hiding this comment.
FYI: Since the scope of the PR has been changed to also support a new version attribute, I believe I'll use this function from other places. Let's wait until that refactor is done to evaluate again.
| } | ||
|
|
||
| if len(responseJSON.Modules) == 0 || len(responseJSON.Modules[0].ModuleVersions) == 0 { | ||
| return "", errors.New(ModuleVersionsFetchErr{sourceURL: url.String()}) |
There was a problem hiding this comment.
It appears that we're reusing the same ModuleVersionsFetchErr error for multiple scenarios. This could be confusing to users, as the same error is thrown when the GET request fails, when the JSON response cannot be parsed, or when no module versions are found.
There was a problem hiding this comment.
While I agree with you, many of those errors are a consequence of the same problem (network issue or lacking proper authentication). As an example, both JSON response parsing error and no ModuleVersions in the response are a consequence of authentication lack OR module not existing, and for certain registries you can't know in advance. As an example, this is a non existing org/module but there's still a partial answer, which is exactly the same as when the org/module exists but the auth is missing or wrong. On the other hand, TFE seems to better implement this for their private registry: at least they return an error. However, since the answers differ from one implementation to another, I consider it's useful to condense these errors into a more abstract one (FetchErr).
What I could do for sure is to create separate errors for the GET request and these other tricky cases, and try to refine as much as possible. Let me know if you have any ideas here, please.
While I agree the new version attribute completely makes sense (and I'm willing to try that implementation), I'm wondering if it belongs to the scope of this PR since the version attribute is already there via query string. I mean, adding that attribute implies a (probably full) refactor of this code and at the end the target is to add the new attribute and not to support version constraints (even if they're related). From my POV the version attribute should be added as an incremental, later change after supporting version constraints. However, that's my take. WDYT? |
juan-vg
changed the title
Normally, I'd be totally onboard with an incremental improvement rather than hold up a PR, but in the case of TG configuration, we want to be more conservative. This introduces configuration for a URL that is not URL encoded, and when we support the attribute, users should switch off using the query string parameter, which would be a breaking change. If you really don't have time to rework with PR to use an attribute, I'd be open to reviewing it after the changes have been gated behind an experiment flag. That way, we can document the feature as incomplete, and require user opt in to use behavior that might break in the future. |
I understand
Nah, let's do it! I have some time and much willing, and any guiding or help will be appreciated :) |
|
Bumping this to see if there were any more updates. We would really like to see this adopted. |
Description
Fixes #1930
Fixes #2572
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added support for version constraints for Terraform Registry modules
Summary by CodeRabbit
New Features
Documentation
VERSIONargument in theterraformblock to clarify acceptable formats.Tests
GetTargetVersionfunction to handle various input scenarios.