Normalize lock file version constraint handling #4541
Conversation
james03160927
requested review from
denis256,
wakeful and
yhakbar
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughA new function for normalizing version constraint strings was added and integrated into the lockfile update logic, ensuring consistent formatting of constraints. The test suite was refactored to allow parameterized mocking of provider constraints, enabling more flexible and thorough testing of constraint handling in the lockfile update process. Changes
Sequence Diagram(s)sequenceDiagram
participant TestCase
participant MockProvider
participant UpdateLockfile
participant normalizeVersionConstraint
TestCase->>MockProvider: Create mock with address, version, constraint
TestCase->>UpdateLockfile: Call UpdateLockfile with mocked providers
UpdateLockfile->>normalizeVersionConstraint: Normalize constraints string
normalizeVersionConstraint-->>UpdateLockfile: Return normalized constraint
UpdateLockfile-->>TestCase: Update lockfile with normalized constraint
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![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 (2)
tf/getproviders/lock_test.go (2)
21-21: Fix parameter name spelling.The parameter name
constrainshould beconstraintto match standard terminology and avoid confusion.Apply this diff:
-func mockProviderUpdateLock(t *testing.T, ctrl *gomock.Controller, address, version, constrain string) getproviders.Provider { +func mockProviderUpdateLock(t *testing.T, ctrl *gomock.Controller, address, version, constraint string) getproviders.Provider {
46-46: Update parameter usage to match corrected name.The parameter usage should be updated to match the corrected parameter name.
Apply this diff:
- provider.EXPECT().Constraints().Return(constrain).AnyTimes() + provider.EXPECT().Constraints().Return(constraint).AnyTimes()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
tf/getproviders/lock.go(3 hunks)tf/getproviders/lock_test.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (1)
tf/getproviders/lock.go (1)
Learnt from: wakeful
PR: gruntwork-io/terragrunt#4491
File: test/integration_test.go:4265-4265
Timestamp: 2025-07-03T22:05:07.356Z
Learning: Constants defined in one Go file are accessible from other files in the same package without explicit imports. When reviewing Go code, consider the package-level scope rather than just the individual file scope.
🧬 Code Graph Analysis (1)
tf/getproviders/lock_test.go (2)
tf/getproviders/provider.go (1)
Provider(12-30)tf/getproviders/lock.go (1)
UpdateLockfile(28-58)
🪛 GitHub Actions: Codespell
tf/getproviders/lock.go
[error] 303-303: codespell: 'contrains' is a misspelling; suggested corrections are 'constrains', 'contains', 'constraints'.
[error] 304-304: codespell: 'contrains' is a misspelling; suggested corrections are 'constrains', 'contains', 'constraints'.
[error] 306-306: codespell: 'contrains' is a misspelling; suggested corrections are 'constrains', 'contains', 'constraints'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Test (Mock)
- GitHub Check: Test (Deprecated)
- GitHub Check: Test (Race)
- GitHub Check: Test (AWS Tofu)
- GitHub Check: Test (CAS)
- GitHub Check: Build (windows/386)
- GitHub Check: Test (Windows)
- GitHub Check: Test (AWS with Latest Terraform)
- GitHub Check: Test (AWSGCP)
- GitHub Check: Test (SOPS)
- GitHub Check: Test (AWS with Latest OSS Terraform)
- GitHub Check: Test (Tflint)
- GitHub Check: Build (windows/amd64)
- GitHub Check: Build (windows/386)
- GitHub Check: Build (darwin/amd64)
- GitHub Check: Build (linux/amd64)
- GitHub Check: Build (linux/arm64)
- GitHub Check: Build (darwin/arm64)
- GitHub Check: Build (linux/386)
- GitHub Check: lint
- GitHub Check: lint
🔇 Additional comments (6)
tf/getproviders/lock.go (3)
11-11: LGTM!The addition of the
regexppackage import is appropriate for the new version constraint normalization functionality.
132-132: LGTM!The integration of the
normalizeVersionConstraintfunction into the constraint setting logic ensures consistent formatting of version constraints in lockfiles.
300-300: LGTM!The regex variable correctly uses
version.VersionRegexpRawfrom the HashiCorp go-version library to match version strings, ensuring consistency with Terraform's version parsing.tf/getproviders/lock_test.go (3)
60-60: LGTM!The refactoring to use a
providersFuncfunction enables dynamic provider creation with different constraints for each test case, improving test flexibility and coverage.
82-99: LGTM!The new test case effectively verifies the constraint normalization functionality by testing that
">= 5.37, < 6 "gets normalized to">= 5.37.0, < 6.0.0 ". This validates the core functionality of the PR.
219-219: LGTM!The test execution correctly uses the new
providersFuncpattern to dynamically generate providers and pass them toUpdateLockfile, enabling parameterized testing of constraint handling.Also applies to: 233-233
| func normalizeVersionConstraint(raw string) string { | ||
| contrains := strings.Split(raw, ",") | ||
| result := make([]string, len(contrains)) | ||
|
|
||
| for i, constrain := range contrains { | ||
| pos := versionRegex.FindStringIndex(constrain) | ||
| if pos == nil { | ||
| result[i] = constrain | ||
| continue | ||
| } | ||
| constrainClean := strings.TrimSpace(constrain[pos[0]:pos[1]]) | ||
| v, err := version.NewVersion(constrainClean) | ||
| if err != nil { | ||
| result[i] = constrain | ||
| continue | ||
| } | ||
|
|
||
| result[i] = constrain[0:pos[0]] + v.String() + constrain[pos[0]+len(constrainClean):] | ||
| } | ||
|
|
||
| return strings.Join(result, ",") | ||
| } |
There was a problem hiding this comment.
Fix spelling errors and improve variable naming.
The function has several spelling errors flagged by the pipeline:
Apply this diff to fix the spelling errors:
func normalizeVersionConstraint(raw string) string {
- contrains := strings.Split(raw, ",")
- result := make([]string, len(contrains))
+ constraints := strings.Split(raw, ",")
+ result := make([]string, len(constraints))
- for i, constrain := range contrains {
+ for i, constraint := range constraints {
- pos := versionRegex.FindStringIndex(constrain)
+ pos := versionRegex.FindStringIndex(constraint)
if pos == nil {
- result[i] = constrain
+ result[i] = constraint
continue
}
- constrainClean := strings.TrimSpace(constrain[pos[0]:pos[1]])
+ constraintClean := strings.TrimSpace(constraint[pos[0]:pos[1]])
- v, err := version.NewVersion(constrainClean)
+ v, err := version.NewVersion(constraintClean)
if err != nil {
- result[i] = constrain
+ result[i] = constraint
continue
}
- result[i] = constrain[0:pos[0]] + v.String() + constrain[pos[0]+len(constrainClean):]
+ result[i] = constraint[0:pos[0]] + v.String() + constraint[pos[0]+len(constraintClean):]
}
return strings.Join(result, ",")
}Additional observations about the function logic:
The normalization logic is sound - it preserves operators and whitespace while normalizing only the version numbers themselves. The error handling appropriately falls back to the original constraint segment if parsing fails.
📝 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 normalizeVersionConstraint(raw string) string { | |
| contrains := strings.Split(raw, ",") | |
| result := make([]string, len(contrains)) | |
| for i, constrain := range contrains { | |
| pos := versionRegex.FindStringIndex(constrain) | |
| if pos == nil { | |
| result[i] = constrain | |
| continue | |
| } | |
| constrainClean := strings.TrimSpace(constrain[pos[0]:pos[1]]) | |
| v, err := version.NewVersion(constrainClean) | |
| if err != nil { | |
| result[i] = constrain | |
| continue | |
| } | |
| result[i] = constrain[0:pos[0]] + v.String() + constrain[pos[0]+len(constrainClean):] | |
| } | |
| return strings.Join(result, ",") | |
| } | |
| func normalizeVersionConstraint(raw string) string { | |
| constraints := strings.Split(raw, ",") | |
| result := make([]string, len(constraints)) | |
| for i, constraint := range constraints { | |
| pos := versionRegex.FindStringIndex(constraint) | |
| if pos == nil { | |
| result[i] = constraint | |
| continue | |
| } | |
| constraintClean := strings.TrimSpace(constraint[pos[0]:pos[1]]) | |
| v, err := version.NewVersion(constraintClean) | |
| if err != nil { | |
| result[i] = constraint | |
| continue | |
| } | |
| result[i] = constraint[0:pos[0]] + v.String() + constraint[pos[0]+len(constraintClean):] | |
| } | |
| return strings.Join(result, ",") | |
| } |
🧰 Tools
🪛 GitHub Actions: Codespell
[error] 303-303: codespell: 'contrains' is a misspelling; suggested corrections are 'constrains', 'contains', 'constraints'.
[error] 304-304: codespell: 'contrains' is a misspelling; suggested corrections are 'constrains', 'contains', 'constraints'.
[error] 306-306: codespell: 'contrains' is a misspelling; suggested corrections are 'constrains', 'contains', 'constraints'.
🤖 Prompt for AI Agents
In tf/getproviders/lock.go around lines 302 to 323, fix the spelling errors by
renaming the variable "contrains" to "constraints" and "constrain" to
"constraint" throughout the function. This improves code readability and
correctness without changing the existing logic.
|
will this fix #4542 ? @james03160927 can you add a test case for my issue? |
|
Hi @grimm26, I don't think this would entirely fix the issue you posted but I believe it's the first step toward it. |
Summary
Test plan
Summary by CodeRabbit
New Features
Tests