-
-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add performance insights variable #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
""" WalkthroughThe pull request upgrades the DocumentDB cluster module version from Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
|
I'm not sure why these tests are failing as we are currently using this code in our downstream component and it works as intended |
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
|
/terratest |
6bef7f9 to
abdd964
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/component_test.go (1)
22-87: Consider adding test coverage for performance insights functionality.While the test improvements are excellent, the PR objectives mention adding performance insights functionality to the DocumentDB module. Consider adding test assertions to verify that performance insights can be enabled and configured correctly.
Would you like me to help generate additional test cases that specifically verify the performance insights functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/component_test.go(5 hunks)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (9)
test/component_test.go (9)
5-5: LGTM! Import addition supports string formatting.The
fmtimport is correctly added to support thefmt.Sprintfusage in the test inputs.
28-28: LGTM! Password variable addition supports master password testing.The addition of a randomly generated password variable aligns with the new
manage_master_user_passwordfunctionality mentioned in the PR objectives.
31-31: LGTM! Type improvement usinganyinstead ofinterface{}.Using
any(alias forinterface{}introduced in Go 1.18) is more concise and idiomatic in modern Go code.
33-33: LGTM! Master password input addition supports new functionality.The addition of
master_passwordto the test inputs enables testing of the new master user password management feature.
34-34: LGTM! Improved string formatting using fmt.Sprintf.Using
fmt.Sprintffor string formatting is cleaner and more readable than string concatenation.
62-63: LGTM! Variable naming consistency improvement.Changing from camelCase
securityGroupIdto PascalCasesecurityGroupIDfollows Go naming conventions where acronyms should be capitalized.
79-79: LGTM! Consistent variable naming with previous changes.The variable name change maintains consistency with the earlier
securityGroupIDnaming improvements.
82-83: LGTM! Variable naming consistency for DNS and ID acronyms.Changing from camelCase
delegatedDnsZoneIdto PascalCasedelegatedDNSZoneIDfollows Go naming conventions for acronyms.
103-104: LGTM! Consistent type improvements usingany.The type changes maintain consistency with the earlier improvements, using
anyinstead ofinterface{}for better readability.
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
|
This improvement is stalled because the upstream module has an |
|
/terratest |
|
@goruha This should be good to go now |
oycyc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
These changes were released in v1.537.0. |
what and why
Note
The module already supports this functionality (we just need to pass the variable)
references
Summary by CodeRabbit