[INF-5307] - Add the MongoDB & PostgreSQL Outbox Ably Ingress Rules #187
Conversation
WalkthroughThe changes in this pull request introduce a new resource for managing MongoDB ingress rules within the Ably Terraform provider. Documentation for the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
12 times, most recently
from
e17595b to
1341ff7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (5)
internal/provider/ingress_rules.go (2)
218-218: Typo in error messageThere's a typo in the error message within the
UpdateIngressRulefunction on line 218. The word "updading" should be corrected to "updating".Apply this diff to fix the typo:
resp.Diagnostics.AddError( - fmt.Sprintf("Error updading Resource %s", r.Name()), + fmt.Sprintf("Error updating Resource %s", r.Name()), fmt.Sprintf("Could not update resource %s, unexpected error: %s", r.Name(), err.Error()), )
262-262: Extra quotation mark in error messageIn the
DeleteIngressRulefunction, there is an extra single quote in the error message format string on line 262. This could lead to formatting issues in the displayed error message.Apply this diff to remove the extra quotation mark:
resp.Diagnostics.AddError( - fmt.Sprintf("Error deleting Resource %s'", r.Name()), + fmt.Sprintf("Error deleting Resource %s", r.Name()), fmt.Sprintf("Could not delete resource '%s', unexpected error: %s", r.Name(), err.Error()), )internal/provider/models.go (2)
90-95: Consider adding documentation comments for the type and fieldsThe structure is well-designed and follows the established patterns. Consider adding documentation comments to describe the purpose of the type and its fields, similar to other types in the codebase.
99-107: Consider enhancing the MongoDB target configurationWhile the structure captures the necessary MongoDB configuration fields, consider these improvements:
- Add field documentation to explain the purpose and expected format of each field
- Consider adding validation tags or constraints for fields like
urlto ensure valid MongoDB connection strings- Consider using more specific types for fields like
pipeline(could be JSON/BSON)internal/provider/resource_ably_ingress_rule_mongo.go (1)
71-74: Consider adding MongoDB-specific error handlingThe generic
CreateIngressRulefunction might not handle MongoDB-specific connection issues optimally.Consider wrapping the generic create function with MongoDB-specific error handling:
func (r resourceIngressRuleMongo) Create(ctx context.Context, req tfsdk_resource.CreateRequest, resp *tfsdk_resource.CreateResponse) { - CreateIngressRule[AblyIngressRuleTargetMongo](&r, ctx, req, resp) + CreateIngressRule[AblyIngressRuleTargetMongo](&r, ctx, req, resp) + if resp.Diagnostics.HasError() { + // Enhance error messages for MongoDB-specific issues + for i, diag := range resp.Diagnostics { + if strings.Contains(diag.Detail(), "connection refused") { + resp.Diagnostics[i].Detail = "Failed to connect to MongoDB. Please verify the URL and ensure the server is running." + } + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
docs/resources/ingress_rule_mongodb.md(1 hunks)docs/resources/rule_azure_function.md(2 hunks)examples/playground/rule_http_azure_function.tf(1 hunks)examples/resources/rule_http_azure_function.tf(1 hunks)go.mod(1 hunks)internal/provider/ingress_rules.go(1 hunks)internal/provider/models.go(1 hunks)internal/provider/provider.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo_test.go(1 hunks)internal/provider/resource_ably_namespace_test.go(2 hunks)internal/provider/resource_ably_rule_azure_function_test.go(2 hunks)templates/index.md.tmpl(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/resources/ingress_rule_mongodb.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
🔇 Additional comments (11)
examples/playground/rule_http_azure_function.tf (1)
10-10: Consistent with main example.
This change mirrors the azure_app_id update in the main example file.
docs/resources/rule_azure_function.md (2)
25-25: Documentation updated to match code changes.
The azure_app_id example has been updated to maintain consistency with the implementation.
93-93: Documentation formatting improvement.
The value field description has been properly formatted.
internal/provider/resource_ably_rule_azure_function_test.go (2)
Line range hint 31-83: Test implementation follows best practices.
The test implementation is well-structured with:
- Comprehensive attribute validation
- Coverage of both initial and updated states
- Proper validation of all configuration changes
- Good use of test helper functions
41-41: Verify consistent usage of the new Azure App ID value.
The change from "coms" to "demo" for targetAzureAppId appears correct and matches the updates in related files.
Let's verify this change is consistent across all test files:
Also applies to: 64-64
✅ Verification successful
Change from "coms" to "demo" is consistently applied
The verification shows that:
- No instances of the old "coms" value remain in the codebase
- The new "demo" value is consistently used in both test configurations within
resource_ably_rule_azure_function_test.go - The change is properly applied in both the initial and updated test states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old "coms" Azure App ID
rg -l 'azure_app_id.*"coms"' internal/provider/
Length of output: 49
Script:
#!/bin/bash
# Let's try a broader search for "coms" in the provider directory
rg -i "coms" internal/provider/
# Also search for any Azure App ID patterns
rg -i "azure.*app.*id" internal/provider/
Length of output: 902
Script:
#!/bin/bash
# Let's check the actual test content to see all Azure App ID values
rg -A 5 -B 5 '"demo"' internal/provider/resource_ably_rule_azure_function_test.go
# Also check for any other test files that might be using Azure App IDs
fd -e go -x grep -l "azure.*app.*id" {} \; internal/provider/
Length of output: 1003
internal/provider/provider.go (1)
145-145: LGTM!
The new MongoDB ingress rule resource is correctly integrated into the provider's resource list.
internal/provider/resource_ably_namespace_test.go (1)
Line range hint 94-107: LGTM! Verify batching policy values
The change from a generic policy name to "simple" looks correct. However, let's verify all supported batching policy values.
internal/provider/models.go (2)
80-88: LGTM: IngressRule method implementation is correct
The method follows the established pattern in the codebase for rule type conversion.
97-97: LGTM: Type alias follows established pattern
The type alias is consistent with the existing AblyRule implementation.
go.mod (1)
6-6: Verify the pre-release dependency version
The update to github.com/ably/ably-control-go v0.4.1-0.20241211161828-60786a2a414b uses a pre-release version. While this may be necessary for the MongoDB ingress rule functionality, pre-release versions can potentially introduce instability.
✅ Verification successful
Pre-release version is required for MongoDB ingress functionality
The pre-release version is justified as it contains the necessary MongoDB ingress rule implementation that's being used in the codebase. The changes between v0.4.0 and this version specifically add MongoDB integration rules, and the codebase actively uses this functionality through the ably_ingress_rule_mongodb resource. This is a deliberate and necessary dependency update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's a stable release available and compare commit dates
# Get the latest stable version
curl -s https://api.github.com/repos/ably/ably-control-go/releases/latest | jq -r '.tag_name'
# Get commit info for the specified version
gh api repos/ably/ably-control-go/commits/60786a2a414b --jq '.commit.message'
Length of output: 347
Script:
#!/bin/bash
# Let's check the changes in the pre-release version to understand if it's specifically needed for MongoDB ingress
# Check the diff between v0.4.0 and the specific commit
gh api repos/ably/ably-control-go/compare/v0.4.0...60786a2a414b --jq '.files[].filename'
# Look for MongoDB related changes in the repository at this commit
rg -l "mongodb" --type go
# Check if the code is actually using MongoDB ingress features
rg "mongodb" -A 3 -B 3 --type go
Length of output: 3239
internal/provider/resource_ably_ingress_rule_mongo.go (1)
91-94: LGTM: Import implementation
The import implementation correctly handles the required identifiers for the MongoDB ingress rule.
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
from
8be5958 to
b16dc46
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
examples/playground/main.tf (1)
5-6: Consider using version constraints instead of fixed versionWhile using a local source is fine for development, consider using a version constraint (e.g.,
">= 0.8.0") instead of a fixed version to allow for patch updates while maintaining compatibility.- version = "0.8.0" + version = ">= 0.8.0"examples/playground/ingress_rule_mongodb.tf (1)
8-14: Consider parameterizing the channel nameThe
_ablyChannelvalue is hardcoded. Consider making it configurable for better reusability.pipeline = jsonencode([ { "$set" = { - "_ablyChannel" = "myChannel" + "_ablyChannel" = var.ably_channel_name } } ])internal/provider/resource_ably_ingress_rule_postgres_outbox.go (2)
20-59: Add descriptions for schema attributesThe schema attributes are missing meaningful descriptions, which reduces the usability of the provider. Consider adding descriptive text for each attribute to help users understand their purpose and expected values.
Example improvement for the
urlattribute:"url": { Type: types.StringType, Required: true, - Description: "", + Description: "PostgreSQL connection URL in the format postgres://user:password@host:port/dbname", },
61-61: Complete the resource documentation stringThe documentation string is incomplete and ends abruptly with "Read more at". Consider adding the complete documentation URL or reference.
-"The `ably_ingress_rule_postgres_outbox` resource allows you to create and manage an Ably Ingress rule for PostgreSQL. Read more at " +"The `ably_ingress_rule_postgres_outbox` resource allows you to create and manage an Ably Ingress rule for PostgreSQL. Read more at https://ably.com/docs"internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go (3)
37-60: Implement the update test caseThe update test case is commented out and incomplete. Consider implementing it to ensure the resource can be properly updated.
100-100: Use a test certificate for SSL verificationThe test contains a real SSL certificate. Consider using a test/mock certificate to prevent issues with certificate expiration or revocation.
Create a test helper that generates a self-signed certificate for testing:
func generateTestCertificate() string { // Generate a self-signed certificate for testing // Return PEM-encoded certificate }
71-81: Consider extracting provider configuration to a shared test helperThe provider configuration block is duplicated across test configurations. Consider extracting it to a shared test helper function to improve maintainability.
Create a helper function:
func providerConfig() string { return ` terraform { required_providers { ably = { source = "github.com/ably/ably" } } } provider "ably" {}` }Then use it in the test configuration:
-terraform { - required_providers { - ably = { - source = "github.com/ably/ably" - } - } -} - -provider "ably" {} +%s `, providerConfig())internal/provider/models.go (1)
90-95: Well-designed generic type for extensibilityThe
AblyIngressRuleDecodertype is well-structured with:
- Proper use of terraform framework types
- Generic target field enabling type-safe rule definitions
- Consistent field tagging for terraform schema mapping
This design pattern will facilitate adding new ingress rule types in the future while maintaining type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Makefile(1 hunks)examples/playground/ingress_rule_mongodb.tf(1 hunks)examples/playground/ingress_rule_postgres_outbox.tf(1 hunks)examples/playground/main.tf(1 hunks)internal/provider/ingress_rules.go(1 hunks)internal/provider/models.go(1 hunks)internal/provider/provider.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/provider/ingress_rules.go
🔇 Additional comments (5)
Makefile (1)
6-6: Verify if the version bump from 0.4.3 to 0.8.0 is justified.
The version increment of 4 minor versions seems significant for adding a MongoDB ingress rule. Please ensure this aligns with semantic versioning principles and confirm if there are other unreleased changes or breaking changes that justify this bump.
internal/provider/resource_ably_ingress_rule_postgres_outbox.go (1)
76-94: LGTM! Clean implementation of CRUD operations
The implementation follows good practices by:
- Using generic functions to reduce code duplication
- Following a consistent pattern across all operations
- Properly implementing the Terraform resource interface
internal/provider/provider.go (1)
145-146: LGTM! Proper registration of new resources
The new resources are correctly registered in the provider's resource list, following the established pattern.
internal/provider/models.go (2)
80-88: LGTM! Method follows established patterns
The IngressRule() method implementation is clean and consistent with the existing codebase patterns, particularly matching the structure of the Rule() method.
97-97: LGTM! Consistent type alias definition
The type alias follows the established pattern seen with AblyRule, providing a base type for all ingress rules.
internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go
Outdated
Show resolved
Hide resolved
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
2 times, most recently
from
096d092 to
1b196c5
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/provider/resource_ably_ingress_rule_mongo_test.go (2)
10-58: 🛠️ Refactor suggestionEnhance test coverage
The test implementation needs additional scenarios:
- Error handling for invalid configurations
- Update scenarios for all mutable fields
- Edge cases with complex pipeline configurations
Consider adding these test cases:
// Test invalid MongoDB URL { Config: testAccAblyIngressRuleMongoConfig( app_name, "enabled", "invalid://url", ), ExpectError: regexp.MustCompile(`Invalid MongoDB URL`), }, // Test pipeline update { Config: testAccAblyIngressRuleMongoConfig( app_name, "enabled", "mongodb://user:pass@host:27017", `[{"$match": {"operationType": "insert"}}, {"$set": {"_ablyChannel": "newChannel"}}]`, ), Check: resource.ComposeAggregateTestCheckFunc( resource.TestCheckResourceAttr("ably_ingress_rule_mongodb.rule0", "target.pipeline", `[{"$match": {"operationType": "insert"}}, {"$set": {"_ablyChannel": "newChannel"}}]`), ), },
88-101:⚠️ Potential issueSecurity: Remove hardcoded credentials
The test configuration contains hardcoded MongoDB credentials and connection details.
Apply this diff:
- url = "mongodb://coco:[email protected]:27017" + url = "mongodb://${MONGODB_USER}:${MONGODB_PASSWORD}@${MONGODB_HOST}:27017" - database = "coconut" + database = "test_db" - collection = "coconut" + collection = "test_collection"
🧹 Nitpick comments (6)
docs/resources/ingress_rule_postgres_outbox.md (1)
6-7: Improve resource description clarityThe description contains run-on sentences and could be more concise. Consider restructuring:
- The ably_ingress_rule_postgres_outbox resource Use the Postgres database connector to distribute changes from your Postgres database to end users at scale. It enables you to distribute records using the outbox pattern to large numbers of subscribing clients, in realtime, as the changes occur. + The ably_ingress_rule_postgres_outbox resource uses the Postgres database connector with the outbox pattern to distribute database changes to end users at scale. This enables real-time distribution of records to large numbers of subscribing clients.Also applies to: 11-11
internal/provider/resource_ably_ingress_rule_mongo_test.go (1)
91-98: Improve test pipeline configurationThe current pipeline configuration is overly simplistic. Consider using a more realistic example that demonstrates common use cases.
pipeline = jsonencode([ - { - "$set" = { - "_ablyChannel" = "myChannel" - } - } + { + "$match": { + "operationType": { "$in": ["insert", "update", "delete"] } + } + }, + { + "$set": { + "_ablyChannel": { + "$concat": ["changes.", "$ns.coll"] + } + } + } ])internal/provider/resource_ably_ingress_rule_mongo.go (2)
20-54: Add field validations for URL and Pipeline attributesConsider adding validators to ensure:
- The URL field contains a valid MongoDB connection string
- The Pipeline field contains a valid JSON array of pipeline operations
Example implementation:
"url": { Type: types.StringType, Required: true, Description: "The connection string of your MongoDB instance. (e.g. mongodb://user:[email protected])", Validators: []tfsdk.AttributeValidator{ // Add MongoDB URL format validator stringvalidator.RegexMatches( regexp.MustCompile(`^mongodb(\+srv)?:\/\/.*`), "must be a valid MongoDB connection string", ), }, }, "pipeline": { Type: types.StringType, Required: true, Description: "A MongoDB pipeline to pass to the Change Stream API...", Validators: []tfsdk.AttributeValidator{ // Add JSON array validator stringvalidator.RegexMatches( regexp.MustCompile(`^\[.*\]$`), "must be a valid JSON array of pipeline operations", ), }, },
71-94: Consider adding method documentationWhile the implementation is correct, consider adding detailed documentation for each method explaining:
- Expected parameters
- Return values
- Error conditions
- Example usage
Example:
// Create creates a new MongoDB ingress rule. // It accepts a context and CreateRequest containing the rule configuration. // Returns a CreateResponse with the created rule or error diagnostics. func (r resourceIngressRuleMongo) Create(ctx context.Context, req tfsdk_resource.CreateRequest, resp *tfsdk_resource.CreateResponse) { CreateIngressRule[AblyIngressRuleTargetMongo](&r, ctx, req, resp) }internal/provider/resource_ably_ingress_rule_postgres_outbox.go (1)
45-54: Enhance SSL mode validation and documentationThe
ssl_modefield should have stricter validation to ensure only valid modes are accepted.Add validation and improve documentation:
"ssl_mode": { Type: types.StringType, Required: true, Description: "Determines the level of protection provided by the SSL connection. Options are: prefer, require, verify-ca, verify-full; default value is prefer.", Validators: []tfsdk.AttributeValidator{ stringvalidator.OneOf( "prefer", "require", "verify-ca", "verify-full", ), }, },internal/provider/ingress_rules.go (1)
238-241: Fix typo in error messageThere's a typo in the error message for updating resources.
- fmt.Sprintf("Error updading Resource %s", r.Name()), + fmt.Sprintf("Error updating Resource %s", r.Name()),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
Makefile(2 hunks)docs/resources/ingress_rule_mongodb.md(1 hunks)docs/resources/ingress_rule_postgres_outbox.md(1 hunks)docs/resources/rule_azure_function.md(1 hunks)examples/playground/ingress_rule_mongodb.tf(1 hunks)examples/playground/ingress_rule_postgres_outbox.tf(1 hunks)examples/playground/main.tf(1 hunks)examples/playground/rule_http_azure_function.tf(1 hunks)examples/resources/rule_http_azure_function.tf(1 hunks)go.mod(1 hunks)internal/provider/ingress_rules.go(1 hunks)internal/provider/models.go(1 hunks)internal/provider/provider.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo_test.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go(1 hunks)internal/provider/resource_ably_namespace_test.go(2 hunks)internal/provider/resource_ably_rule_azure_function_test.go(2 hunks)templates/index.md.tmpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- examples/resources/rule_http_azure_function.tf
- go.mod
- examples/playground/main.tf
- internal/provider/resource_ably_namespace_test.go
- examples/playground/rule_http_azure_function.tf
- examples/playground/ingress_rule_postgres_outbox.tf
- docs/resources/rule_azure_function.md
- examples/playground/ingress_rule_mongodb.tf
- internal/provider/provider.go
- internal/provider/resource_ably_rule_azure_function_test.go
- internal/provider/models.go
- internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go
- templates/index.md.tmpl
🧰 Additional context used
🪛 LanguageTool
docs/resources/ingress_rule_postgres_outbox.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
[grammar] ~41-~41: Consider using either the past participle “preferred” or the present participle “preferring” here.
Context: ...erify-ca, verify-full; default value is prefer. - url (String) The URL for your Post...
(BEEN_PART_AGREEMENT)
docs/resources/ingress_rule_mongodb.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
🔇 Additional comments (8)
Makefile (4)
7-15: Architecture detection implementation looks good!
The implementation properly handles architecture detection with robust error checking and comprehensive architecture mappings.
24-24: ARM64 build targets added correctly!
The release target now properly includes arm64 builds for both Darwin and Linux platforms.
Also applies to: 30-30
Line range hint 39-40: Verify the install target behavior with different architectures.
The install target now uses the dynamic ${OS_ARCH} variable. Please ensure this works correctly across different platforms, especially with the newly added arm64 support.
✅ Verification successful
The previous search didn't yield results because we used literal variable names. Let's examine the Makefile and documentation to understand how the installation paths and architecture support are implemented.
After examining the Makefile and documentation, I can now provide a definitive response about the install target's architecture support:
The install target's architecture handling is correctly implemented and safe.
The Makefile includes proper architecture mapping and validation:
- Native architecture is detected via
uname -m - Architecture is mapped correctly:
- x86_64 → amd64
- aarch64 → arm64
- armv7l → arm
- Error checking is implemented to prevent installation with unsupported architectures
- The mapped architecture is used consistently in the install path
The release target also shows that all supported architectures are properly built:
- amd64 and arm64 for Darwin
- 386, amd64, and arm for FreeBSD/Linux/OpenBSD
- amd64 for Solaris
- 386 and amd64 for Windows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the install paths are referenced correctly in documentation and examples
rg "terraform\.d/plugins/${HOSTNAME}/${NAMESPACE}/${NAME}" --type md
rg "\.terraform\.d/plugins" --type tf
Length of output: 89
Script:
#!/bin/bash
# Let's check the Makefile for OS_ARCH definition and usage
cat Makefile
# Also check for any documentation files that might contain installation instructions
fd -e md -X cat
Length of output: 70647
6-6: Verify the rationale for the significant version bump.
The version change from 0.4.3 to 0.8.0 is substantial. Please ensure this aligns with semantic versioning principles and document any breaking changes.
docs/resources/ingress_rule_mongodb.md (2)
11-14: Documentation needs enhancement
The resource description should include:
- Link to MongoDB Change Streams documentation
- Example configuration showing practical usage
- Common use cases and limitations
36-42: Enhance target field documentation with examples
The target fields would benefit from example values and additional context:
collection: Add example format (e.g., "users", "transactions")database: Add example namepipeline: Add example pipeline configurationprimary_site: List available site options
internal/provider/resource_ably_ingress_rule_mongo.go (1)
59-69: LGTM!
The resource metadata implementation is correct and follows Terraform provider development best practices.
internal/provider/ingress_rules.go (1)
16-49:
Add default case in GetPlanIngressRule switch statement
The switch statement should handle unknown target types to prevent potential nil pointer dereferences.
Add a default case:
switch t := plan.Target.(type) {
case *AblyIngressRuleTargetMongo:
// ... existing code ...
case *AblyIngressRuleTargetPostgresOutbox:
// ... existing code ...
+default:
+ panic(fmt.Sprintf("Unsupported target type: %T", t))
}Likely invalid or redundant comment.
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
4 times, most recently
from
697ba87 to
ef96093
Compare
|
@coderabbitai review |
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
4 times, most recently
from
fdbb431 to
84db180
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
docs/resources/ingress_rule_mongodb.md (1)
38-40: Enhance field descriptions for MongoDB configurationThe descriptions for
full_documentandfull_document_before_changefields need improvement:
- Add examples of when each option should be used
- Explain performance implications of different settings
- Clarify default values in a separate line
🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: The preposition “for” seems more likely in this position.
Context: ...Document before change is not available on all types of change event. Possible val...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
docs/resources/ingress_rule_postgres_outbox.md (3)
11-11: Fix grammatical error in resource descriptionThe sentence structure needs improvement:
-The `ably_ingress_rule_postgres_outbox` resource Use the Postgres database connector +The `ably_ingress_rule_postgres_outbox` resource uses the Postgres database connector
48-48: Enhance database URL example with security best practicesThe current URL example includes password in plaintext. Consider:
- Using environment variables for sensitive information
- Adding a note about secure credential management
- Mentioning support for connection string parameters
-The URL for your Postgres database, for example postgres://user:[email protected]:5432/your-database-name. +The URL for your Postgres database, for example postgres://${DB_USER}:${DB_PASSWORD}@example.com:5432/your-database-name. +Environment variables should be used for sensitive information in production environments.
39-39: Fix missing comma in descriptionAdd a comma for better readability:
-Schema for the outbox table in your database which allows for +Schema for the outbox table in your database, which allows for🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...ng) Schema for the outbox table in your database which allows for the reliable publicati...(AI_HYDRA_LEO_MISSING_COMMA)
internal/provider/ingress_rules.go (4)
43-46: Use consistent naming conventionThe variable name
rule_valuesuses snake_case instead of the Go convention of camelCase.Apply this diff:
- rule_values := ably_control_go.NewIngressRule{ + ruleValues := ably_control_go.NewIngressRule{ Status: plan.Status.ValueString(), Target: target, }
239-240: Fix typo in error messageThere's a typo in the error message: "updading" should be "updating".
Apply this diff:
resp.Diagnostics.AddError( - fmt.Sprintf("Error updading Resource %s", r.Name()), + fmt.Sprintf("Error updating Resource %s", r.Name()), fmt.Sprintf("Could not update resource %s, unexpected error: %s", r.Name(), err.Error()), )
283-284: Fix inconsistent error message formattingThe error message has an extra single quote that's inconsistent with other error messages.
Apply this diff:
resp.Diagnostics.AddError( - fmt.Sprintf("Error deleting Resource %s'", r.Name()), + fmt.Sprintf("Error deleting Resource %s", r.Name()), fmt.Sprintf("Could not delete resource '%s', unexpected error: %s", r.Name(), err.Error()), )
278-279: Improve error message consistencyThe error message repeats the resource name twice and has inconsistent formatting.
Apply this diff:
resp.Diagnostics.AddWarning( - fmt.Sprintf("Resource does %s not exist", r.Name()), - fmt.Sprintf("Resource does %s not exist, it may have already been deleted: %s", r.Name(), err.Error()), + fmt.Sprintf("Resource %s does not exist", r.Name()), + fmt.Sprintf("Resource %s does not exist, it may have already been deleted: %s", r.Name(), err.Error()), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/check.yml(1 hunks)Makefile(2 hunks)docs/resources/ingress_rule_mongodb.md(1 hunks)docs/resources/ingress_rule_postgres_outbox.md(1 hunks)docs/resources/rule_azure_function.md(1 hunks)examples/playground/ingress_rule_mongodb.tf(1 hunks)examples/playground/ingress_rule_postgres_outbox.tf(1 hunks)examples/playground/main.tf(1 hunks)examples/playground/rule_http_azure_function.tf(1 hunks)examples/playground/variables.tf(1 hunks)examples/resources/rule_http_azure_function.tf(1 hunks)go.mod(1 hunks)internal/provider/ingress_rules.go(1 hunks)internal/provider/models.go(1 hunks)internal/provider/provider.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo_test.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go(1 hunks)internal/provider/resource_ably_namespace_test.go(2 hunks)internal/provider/resource_ably_rule_azure_function_test.go(2 hunks)templates/index.md.tmpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- go.mod
- examples/playground/rule_http_azure_function.tf
- docs/resources/rule_azure_function.md
- internal/provider/resource_ably_rule_azure_function_test.go
- internal/provider/resource_ably_namespace_test.go
- examples/resources/rule_http_azure_function.tf
- templates/index.md.tmpl
- internal/provider/provider.go
- examples/playground/main.tf
- examples/playground/ingress_rule_postgres_outbox.tf
- examples/playground/ingress_rule_mongodb.tf
- examples/playground/variables.tf
- internal/provider/models.go
- internal/provider/resource_ably_ingress_rule_postgres_outbox.go
👮 Files not reviewed due to content moderation or server errors (3)
- internal/provider/resource_ably_ingress_rule_mongo.go
- internal/provider/resource_ably_ingress_rule_mongo_test.go
- internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go
🧰 Additional context used
🪛 LanguageTool
docs/resources/ingress_rule_mongodb.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
[uncategorized] ~39-~39: The preposition “for” seems more likely in this position.
Context: ...Document before change is not available on all types of change event. Possible val...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
docs/resources/ingress_rule_postgres_outbox.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...ng) Schema for the outbox table in your database which allows for the reliable publicati...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (7)
.github/workflows/check.yml (1)
17-17: Verify dependency compatibility with Go 1.22
Since you're upgrading to Go 1.22 and updating the ably-control-go dependency, let's verify compatibility.
docs/resources/ingress_rule_mongodb.md (1)
6-6: Documentation needs enhancement with examples and use cases
While the description explains the basic purpose, it would be more helpful to include:
- Example configuration showing practical usage
- Common use cases and scenarios
- Link to Ably's MongoDB integration documentation
Also applies to: 11-11
Makefile (2)
6-6: Version bump from 0.4.3 to 0.8.0 needs justification
The version jump appears to skip several versions (0.5.0, 0.6.0, 0.6.1, 0.7.0) based on the project's version history. Consider:
- Using 0.7.1 for minor feature additions
- Using 0.8.0 only if this change introduces breaking changes
24-24: LGTM: Good addition of arm64 support
The addition of arm64 build targets for both Darwin and Linux platforms improves cross-platform compatibility.
Also applies to: 30-30
internal/provider/ingress_rules.go (3)
90-122: LGTM! Well-structured schema definition
The schema is well-defined with appropriate attribute types, descriptions, and plan modifiers.
295-318: LGTM! Well-implemented import functionality
The import functionality includes proper validation of the import identifier format and handles errors appropriately.
16-49: 🛠️ Refactor suggestion
Add validation and improve error handling in GetPlanIngressRule
The function needs additional safeguards:
- Validate required fields before conversion
- Handle unsupported target types
Apply this diff:
func GetPlanIngressRule(plan AblyIngressRule) ably_control_go.NewIngressRule {
+ if plan.Status.IsNull() {
+ panic("Status is required")
+ }
+ if plan.Target == nil {
+ panic("Target is required")
+ }
var target ably_control_go.IngressTarget
switch t := plan.Target.(type) {
// ... existing cases ...
+ default:
+ panic(fmt.Sprintf("Unsupported target type: %T", t))
}
// ... rest of the function ...
}Likely invalid or redundant comment.
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
from
84db180 to
bfd7aca
Compare
graham-russell
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
Makefile (1)
8-8: Enhance the comment descriptionThe current comment "Add error checking for supported architectures" could be more descriptive. Consider updating it to better explain the purpose and supported architectures.
-# Add error checking for supported architectures +# Validate system architecture (supported: amd64, arm64, arm)internal/provider/resource_ably_ingress_rule_mongo.go (1)
47-48: Typographical Error in 'full_document_before_change' DescriptionThere is a typographical error in the description of the
full_document_before_changefield: "wheter" should be "whether".Apply this fix:
- Description: "Controls wheter the full document before the change should be included in the change event. Full Document before change is not available on all types of change event. Possible values are whenAvailable or off. The default is off.", + Description: "Controls whether the full document before the change should be included in the change event. Full Document before change is not available on all types of change event. Possible values are whenAvailable or off. The default is off.",docs/resources/ingress_rule_mongodb.md (1)
36-42: Enhance target field documentationThe target fields need more detailed descriptions:
collection: Add format requirements and examplesdatabase: Specify naming constraintspipeline: Include example JSON array structureprimary_site: List available site optionsExample enhancement for pipeline field:
- `pipeline` (String) A MongoDB pipeline to pass to the Change Stream API. This field allows you to control which types of change events are published, and which channel the change event should be published to. The pipeline must set the _ablyChannel field on the root of the change event. It must also be a valid JSON array of pipeline operations. + `pipeline` (String) A MongoDB pipeline to pass to the Change Stream API. Controls which change events are published and to which channel. Requirements: + - Must be a valid JSON array of pipeline operations + - Must set `_ablyChannel` field on the root of change event + Example: `[{"$match": {"operationType": "insert"}}, {"$set": {"_ablyChannel": "myChannel"}}]`docs/resources/ingress_rule_postgres_outbox.md (2)
6-6: Improve description readabilityThe description contains a run-on sentence and grammatical issues.
- The ably_ingress_rule_postgres_outbox resource Use the Postgres database connector to distribute changes from your Postgres database to end users at scale. It enables you to distribute records using the outbox pattern to large numbers of subscribing clients, in realtime, as the changes occur. + The ably_ingress_rule_postgres_outbox resource uses the Postgres database connector to distribute changes from your Postgres database to end users at scale. It enables you to distribute records using the outbox pattern to large numbers of subscribing clients in real-time as changes occur.
39-39: Fix missing comma in descriptionAdd a comma after "database" for better readability.
- Schema for the outbox table in your database which allows for the reliable publication + Schema for the outbox table in your database, which allows for the reliable publication🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...ng) Schema for the outbox table in your database which allows for the reliable publicati...(AI_HYDRA_LEO_MISSING_COMMA)
internal/provider/resource_ably_ingress_rule_mongo_test.go (1)
90-91: Add provider configuration documentationAdd a comment explaining the required environment variables format and any other configuration prerequisites.
-# You can provide your Ably Token & URL inline or use environment variables ABLY_ACCOUNT_TOKEN & ABLY_URL -provider "ably" {} +# Provider Configuration +# Required environment variables: +# - ABLY_ACCOUNT_TOKEN: Your Ably account token +# - ABLY_URL: Your Ably API endpoint (optional) +# Example: +# export ABLY_ACCOUNT_TOKEN="your-token-here" +# export ABLY_URL="https://api.ably.io" +provider "ably" {}internal/provider/ingress_rules.go (2)
239-240: Fix typo in error message.There's a typo in the error message: "updading" should be "updating".
Apply this diff:
- fmt.Sprintf("Error updading Resource %s", r.Name()), + fmt.Sprintf("Error updating Resource %s", r.Name()),
278-279: Fix grammatical error in warning message.The warning message has a grammatical error with duplicate "does".
Apply this diff:
- fmt.Sprintf("Resource does %s not exist", r.Name()), - fmt.Sprintf("Resource does %s not exist, it may have already been deleted: %s", r.Name(), err.Error()), + fmt.Sprintf("Resource %s does not exist", r.Name()), + fmt.Sprintf("Resource %s does not exist, it may have already been deleted: %s", r.Name(), err.Error()),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/check.yml(1 hunks)Makefile(2 hunks)docs/resources/ingress_rule_mongodb.md(1 hunks)docs/resources/ingress_rule_postgres_outbox.md(1 hunks)docs/resources/rule_azure_function.md(1 hunks)examples/playground/ingress_rule_mongodb.tf(1 hunks)examples/playground/ingress_rule_postgres_outbox.tf(1 hunks)examples/playground/main.tf(1 hunks)examples/playground/rule_http_azure_function.tf(1 hunks)examples/playground/variables.tf(1 hunks)examples/resources/rule_http_azure_function.tf(1 hunks)go.mod(1 hunks)internal/provider/ingress_rules.go(1 hunks)internal/provider/models.go(1 hunks)internal/provider/provider.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo_test.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go(1 hunks)internal/provider/resource_ably_namespace_test.go(2 hunks)internal/provider/resource_ably_rule_azure_function_test.go(2 hunks)templates/index.md.tmpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- docs/resources/rule_azure_function.md
- examples/playground/rule_http_azure_function.tf
- go.mod
- .github/workflows/check.yml
- internal/provider/resource_ably_namespace_test.go
- templates/index.md.tmpl
- examples/playground/main.tf
- internal/provider/resource_ably_rule_azure_function_test.go
- examples/resources/rule_http_azure_function.tf
- examples/playground/ingress_rule_mongodb.tf
- internal/provider/provider.go
- examples/playground/ingress_rule_postgres_outbox.tf
- internal/provider/models.go
- examples/playground/variables.tf
🧰 Additional context used
🪛 LanguageTool
docs/resources/ingress_rule_mongodb.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
docs/resources/ingress_rule_postgres_outbox.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...ng) Schema for the outbox table in your database which allows for the reliable publicati...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (11)
Makefile (2)
6-6: Version bump from 0.4.3 to 0.8.0 appears to be incorrect
Based on the CHANGELOG.md, the project has already progressed through versions 0.5.0, 0.6.0, 0.6.1, and 0.7.0 with documented changes. The version bump to 0.8.0 in the Makefile should be 0.7.1 or 0.7.0 to align with the project's version history.
24-24: LGTM: Added arm64 build targets
The addition of arm64 build targets for Darwin and Linux platforms is well-implemented and follows the existing pattern consistently.
Also applies to: 30-30
internal/provider/resource_ably_ingress_rule_mongo.go (1)
56-56: Include Documentation URL in Resource Description
Consider adding a link to the official documentation in the resource description to provide users with additional context and guidance.
docs/resources/ingress_rule_mongodb.md (1)
6-6: Documentation needs completion
The resource description is incomplete:
- Add a "Read more" link to the MongoDB integration documentation
- Include example usage showing a complete configuration
Also applies to: 11-11
internal/provider/resource_ably_ingress_rule_mongo_test.go (1)
13-14:
Security: Remove hardcoded credentials from test configuration
The MongoDB connection URLs contain hardcoded credentials. Use environment variables instead.
- update_mongo_url := "mongodb://me:[email protected]:27017"
- test_mongo_url := "mongodb://coco:[email protected]:27017"
+ update_mongo_url := fmt.Sprintf("mongodb://%s:%s@%s:27017",
+ os.Getenv("TEST_MONGODB_USER"),
+ os.Getenv("TEST_MONGODB_PASSWORD"),
+ os.Getenv("TEST_MONGODB_HOST"))
+ test_mongo_url := fmt.Sprintf("mongodb://%s:%s@%s:27017",
+ os.Getenv("TEST_MONGODB_USER"),
+ os.Getenv("TEST_MONGODB_PASSWORD"),
+ os.Getenv("TEST_MONGODB_HOST"))Likely invalid or redundant comment.
internal/provider/resource_ably_ingress_rule_postgres_outbox.go (1)
1-105: LGTM! Well-structured resource implementation.
The implementation follows Terraform provider development best practices with proper schema validation and resource lifecycle management.
internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go (3)
13-14: Use environment variables for sensitive data.
The test configuration contains hardcoded database credentials in the URLs. This is a security risk and could lead to accidental commits of sensitive data.
31-31: Security: Avoid hardcoding SSL certificates in tests.
The SSL root certificate is hardcoded in multiple places in the test configuration.
Also applies to: 43-43, 58-58, 70-70
16-76: Enhance test coverage with additional scenarios.
The test implementation should include more scenarios:
- Error handling for invalid configurations
- Different SSL modes
- Connection failures
- Invalid table schemas
internal/provider/ingress_rules.go (2)
19-41:
Add default case to handle unsupported target types.
The type switch for plan.Target lacks a default case. If an unsupported target type is provided, target will remain nil, potentially causing issues later.
Apply this diff:
switch t := plan.Target.(type) {
case *AblyIngressRuleTargetMongo:
// ... existing code ...
case *AblyIngressRuleTargetPostgresOutbox:
// ... existing code ...
+default:
+ panic(fmt.Sprintf("Unsupported target type: %T", t))
}Likely invalid or redundant comment.
56-78:
Add default case to handle unsupported response target types.
Similar to the plan conversion, the response handling switch statement lacks a default case for unsupported target types.
Apply this diff:
switch v := ably_ingress_rule.Target.(type) {
case *ably_control_go.IngressMongoTarget:
// ... existing code ...
case *ably_control_go.IngressPostgresOutboxTarget:
// ... existing code ...
+default:
+ panic(fmt.Sprintf("Unsupported target type in response: %T", v))
}Likely invalid or redundant comment.
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
from
bfd7aca to
b80045b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
docs/resources/ingress_rule_mongodb.md (1)
39-39: Fix typo in 'full_document_before_change' descriptionThere's a typo in the description of the
full_document_before_changefield: "wheter" should be "whether".Apply this diff to correct the typo:
- - `full_document_before_change` (String) Controls wheter the full document before the change should be included in the change event. Full Document before change is not available on all types of change event. Possible values are whenAvailable or off. The default is off. + - `full_document_before_change` (String) Controls whether the full document before the change should be included in the change event. Full Document before change is not available on all types of change event. Possible values are whenAvailable or off. The default is off.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: The preposition “for” seems more likely in this position.
Context: ...Document before change is not available on all types of change event. Possible val...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
docs/resources/ingress_rule_postgres_outbox.md (1)
39-39: Add missing comma in 'outbox_table_schema' descriptionPlease add a comma after "database" to improve readability.
Apply this diff:
- - `outbox_table_schema` (String) Schema for the outbox table in your database which allows for the reliable publication of an ordered sequence of change event messages over Ably. + - `outbox_table_schema` (String) Schema for the outbox table in your database, which allows for the reliable publication of an ordered sequence of change event messages over Ably.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...ng) Schema for the outbox table in your database which allows for the reliable publicati...(AI_HYDRA_LEO_MISSING_COMMA)
internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go (1)
47-73: Split update test into focused test casesConsider breaking down the update test into smaller, focused test cases that validate individual field updates. This would:
- Make test failures more specific
- Improve test maintainability
- Make it easier to identify which changes cause issues
internal/provider/ingress_rules.go (1)
239-240: Fix typo in error messageThere's a typo in the error message ("updading" instead of "updating").
Apply this diff:
- fmt.Sprintf("Error updading Resource %s", r.Name()), + fmt.Sprintf("Error updating Resource %s", r.Name()),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
.github/workflows/check.yml(1 hunks)COPYRIGHT(1 hunks)Makefile(2 hunks)docs/resources/ingress_rule_mongodb.md(1 hunks)docs/resources/ingress_rule_postgres_outbox.md(1 hunks)docs/resources/rule_azure_function.md(1 hunks)examples/playground/ingress_rule_mongodb.tf(1 hunks)examples/playground/ingress_rule_postgres_outbox.tf(1 hunks)examples/playground/main.tf(1 hunks)examples/playground/rule_http_azure_function.tf(1 hunks)examples/playground/variables.tf(1 hunks)examples/resources/rule_http_azure_function.tf(1 hunks)go.mod(1 hunks)internal/provider/ingress_rules.go(1 hunks)internal/provider/models.go(1 hunks)internal/provider/provider.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo_test.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go(1 hunks)internal/provider/resource_ably_namespace_test.go(2 hunks)internal/provider/resource_ably_rule_azure_function_test.go(2 hunks)templates/index.md.tmpl(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- COPYRIGHT
🚧 Files skipped from review as they are similar to previous changes (16)
- examples/playground/rule_http_azure_function.tf
- .github/workflows/check.yml
- go.mod
- examples/playground/main.tf
- docs/resources/rule_azure_function.md
- internal/provider/resource_ably_namespace_test.go
- examples/playground/ingress_rule_postgres_outbox.tf
- internal/provider/resource_ably_rule_azure_function_test.go
- internal/provider/provider.go
- internal/provider/models.go
- examples/playground/ingress_rule_mongodb.tf
- examples/resources/rule_http_azure_function.tf
- examples/playground/variables.tf
- templates/index.md.tmpl
- internal/provider/resource_ably_ingress_rule_mongo_test.go
- internal/provider/resource_ably_ingress_rule_postgres_outbox.go
🧰 Additional context used
🪛 LanguageTool
docs/resources/ingress_rule_mongodb.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
[uncategorized] ~39-~39: The preposition “for” seems more likely in this position.
Context: ...Document before change is not available on all types of change event. Possible val...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
docs/resources/ingress_rule_postgres_outbox.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...ng) Schema for the outbox table in your database which allows for the reliable publicati...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (9)
internal/provider/resource_ably_ingress_rule_mongo.go (1)
21-54: Add Validators to Schema Fields for Enhanced Data Integrity
As previously mentioned, the schema fields currently lack validators, which help enforce correct data formats and provide immediate user feedback on errors. Consider adding appropriate validators to ensure data integrity:
- url: Validate that the provided string is a valid MongoDB connection URL.
- pipeline: Verify that the pipeline is a valid JSON array of pipeline operations.
- full_document and full_document_before_change: Ensure that the values are within the expected set of options (e.g.,
updateLookup,whenAvailable,off). - primary_site: Validate against a list of supported sites to prevent invalid entries.
docs/resources/ingress_rule_mongodb.md (1)
6-7: Include a reference link in the resource description
As previously mentioned, consider adding a "Read more at" link in the resource description to provide users with quick access to detailed documentation.
Makefile (3)
6-6: Verify the significant version bump
The version has been increased from 0.4.3 to 0.8.0, which is a substantial jump. Please ensure this aligns with semantic versioning practices and the scope of changes in this PR.
11-13: LGTM
The architecture detection logic has been corrected and now properly handles unsupported architectures.
24-30: Addition of ARM64 Build Targets
Great job adding ARM64 build targets for both Darwin and Linux platforms. This enhances compatibility for users on different architectures.
internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go (2)
13-14: Use test constants or environment variables for sensitive data
The test configuration contains hardcoded database credentials and connection details.
31-31: Security: Avoid hardcoding SSL certificates
The SSL root certificate is hardcoded in the test configuration.
Also applies to: 43-43
internal/provider/ingress_rules.go (2)
124-127: LGTM! Clean and type-safe implementation
The resource management implementation using generics and the import functionality is well-structured and type-safe.
Also applies to: 294-318
19-41:
Add default case in type switch
The type switch for plan.Target doesn't handle unknown target types, which could lead to a nil target being used.
Apply this diff:
switch t := plan.Target.(type) {
case *AblyIngressRuleTargetMongo:
// ... existing code ...
case *AblyIngressRuleTargetPostgresOutbox:
// ... existing code ...
+default:
+ panic(fmt.Sprintf("Unsupported target type: %T", t))
}Likely invalid or redundant comment.
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
2 times, most recently
from
0521e8e to
700a7c9
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/provider/ingress_rules.go (2)
19-41:⚠️ Potential issueAdd default case to handle unsupported target types
The switch statement lacks a default case, which could result in a nil target if an unsupported type is provided.
Apply this diff to add proper error handling:
switch t := plan.Target.(type) { case *AblyIngressRuleTargetMongo: target = &ably_control_go.IngressMongoTarget{ // ... existing code ... } case *AblyIngressRuleTargetPostgresOutbox: target = &ably_control_go.IngressPostgresOutboxTarget{ // ... existing code ... } +default: + panic(fmt.Sprintf("Unsupported target type: %T", t)) }
56-78:⚠️ Potential issueAdd default case to handle unsupported response target types
The switch statement lacks a default case, which could result in a nil resp_target if an unsupported type is received from the API.
Apply this diff to add proper error handling:
switch v := ably_ingress_rule.Target.(type) { case *ably_control_go.IngressMongoTarget: resp_target = &AblyIngressRuleTargetMongo{ // ... existing code ... } case *ably_control_go.IngressPostgresOutboxTarget: resp_target = &AblyIngressRuleTargetPostgresOutbox{ // ... existing code ... } +default: + panic(fmt.Sprintf("Unsupported target type in response: %T", v)) }
🧹 Nitpick comments (5)
docs/resources/ingress_rule_postgres_outbox.md (2)
41-41: Remove duplicate text in ssl_mode descriptionThere's redundant text in the description: "+ ssl_mode (String)" is repeated.
-- `ssl_mode` (String) + ssl_mode (String) Determines the level of protection provided by the SSL connection. Options are: ++ `ssl_mode` (String) Determines the level of protection provided by the SSL connection. Options are:
38-39: Improve clarity of table schema descriptionsThe descriptions need grammatical improvements:
- Add a comma after "database" in the outbox table description
- Clarify the nodes table's fault tolerance purpose
-Schema for the nodes table in your database to allow for operation as a cluster to provide fault tolerance. +Schema for the nodes table that enables cluster operation and provides fault tolerance. -Schema for the outbox table in your database which allows for the reliable publication +Schema for the outbox table in your database, which allows for the reliable publication🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...ng) Schema for the outbox table in your database which allows for the reliable publicati...(AI_HYDRA_LEO_MISSING_COMMA)
internal/provider/resource_ably_ingress_rule_postgres_outbox.go (1)
64-64: Remove extra tab character in descriptionThe description contains an extra tab character at the beginning.
- Description: " The primary data center in which to run the integration rule.", + Description: "The primary data center in which to run the integration rule.",internal/provider/ingress_rules.go (2)
239-240: Fix typo in error messageThere's a typo in the error message: "updading" should be "updating".
Apply this diff:
- fmt.Sprintf("Error updading Resource %s", r.Name()), + fmt.Sprintf("Error updating Resource %s", r.Name()),
314-314: Remove outdated commentThe comment referencing a "Recent PR" is outdated and should be removed as it's no longer relevant.
Apply this diff:
- // Recent PR in TF Plugin Framework for paths but Hashicorp examples not updated - https://github.com/hashicorp/terraform-plugin-framework/pull/390
🛑 Comments failed to post (3)
internal/provider/resource_ably_ingress_rule_postgres_outbox.go (1)
45-55: 🛠️ Refactor suggestion
Add validation for SSL mode values
The implementation should validate that the SSL mode value is one of the allowed options: "prefer", "require", "verify-ca", or "verify-full".
"ssl_mode": { Type: types.StringType, Required: true, + Validators: []tfsdk.AttributeValidator{ + stringvalidator.OneOf( + "prefer", + "require", + "verify-ca", + "verify-full", + ), + }, Description: `Determines the level of protection provided by the SSL connection...📝 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."ssl_mode": { Type: types.StringType, Required: true, Validators: []tfsdk.AttributeValidator{ stringvalidator.OneOf( "prefer", "require", "verify-ca", "verify-full", ), }, Description: `+ ssl_mode (String) Determines the level of protection provided by the SSL connection. Options are: - prefer: Attempt SSL but allow non-SSL. - require: Always use SSL but don't verify certificates. - verify-ca: Verify server certificate is signed by a trusted CA. - verify-full: Verify server certificate and hostname. Default: prefer.`, },internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go (2)
31-31:
⚠️ Potential issueMove SSL certificates to test fixtures
Hardcoded SSL certificates should be moved to test fixture files for better maintainability and security.
+func loadTestCertificate(t *testing.T) string { + certBytes, err := os.ReadFile("testdata/test_cert.pem") + if err != nil { + t.Fatal(err) + } + return string(certBytes) +}Then update the test configuration:
- "-----BEGIN CERTIFICATE----- MIIFiTCCA3GgAwIBAgIUYO1Lomxzj7VRawWwEFiQht9OLpUwDQYJKoZIhvcNAQEL...", + loadTestCertificate(t),Also applies to: 57-58
16-76: 🛠️ Refactor suggestion
Enhance test coverage with error scenarios
The test suite only covers the happy path. Add test cases for:
- Invalid SSL modes
- Missing required fields
- Invalid database URLs
- Connection failures
- Invalid table names
Example additional test case:
{ Config: testAccAblyIngressRulePostgresOutboxConfig( app_name, "enabled", "invalid://url", "public", "outbox", "public", "nodes", "prefer", "", "us-east-1-A", ), ExpectError: regexp.MustCompile(`Invalid database URL`), },
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
from
700a7c9 to
a3005b9
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
♻️ Duplicate comments (4)
internal/provider/ingress_rules.go (1)
19-41:⚠️ Potential issueAdd default case to handle unknown target types
The type switch lacks a default case, which could lead to a nil
targetif an unknown type is provided.Add a default case to handle unknown types:
switch t := plan.Target.(type) { case *AblyIngressRuleTargetMongo: // ... existing code ... case *AblyIngressRuleTargetPostgresOutbox: // ... existing code ... +default: + panic(fmt.Sprintf("Unsupported target type: %T", t)) }internal/provider/resource_ably_ingress_rule_mongo.go (3)
20-24: 🛠️ Refactor suggestionConsider making URL validation more robust
While the description is clear, the URL field would benefit from validation to ensure it's a valid MongoDB connection string.
35-39: 🛠️ Refactor suggestionAdd JSON validation for pipeline field
The pipeline field requires a valid JSON array of pipeline operations. Consider adding JSON format validation to catch errors early.
40-44: 🛠️ Refactor suggestionAdd enum validation for document configuration fields
The
full_documentandfull_document_before_changefields accept specific values. Consider adding enum validation to prevent invalid inputs.Also applies to: 45-49
🧹 Nitpick comments (5)
docs/resources/ingress_rule_mongodb.md (1)
38-40: Improve field documentation with examplesThe
full_documentfield description would benefit from:
- Example values for each option (
updateLookup,whenAvailable,off)- Use cases for each option
- Impact on performance/resource usage
internal/provider/ingress_rules.go (2)
238-241: Fix typo in error messageThere's a typo in the error message ("updading" instead of "updating").
Apply this fix:
- fmt.Sprintf("Error updading Resource %s", r.Name()), + fmt.Sprintf("Error updating Resource %s", r.Name()),
278-280: Fix inconsistent error message formattingThe error message has inconsistent string formatting with repeated placeholder.
Apply this fix:
- fmt.Sprintf("Resource does %s not exist", r.Name()), - fmt.Sprintf("Resource does %s not exist, it may have already been deleted: %s", r.Name(), err.Error()), + fmt.Sprintf("Resource '%s' does not exist", r.Name()), + fmt.Sprintf("Resource '%s' does not exist, it may have already been deleted: %s", r.Name(), err.Error()),docs/resources/ingress_rule_postgres_outbox.md (1)
Line range hint
64-64: Remove extra indentation in primary_site descriptionThe description has unnecessary leading whitespace.
Apply this diff to fix the formatting:
- Description: " The primary data center in which to run the integration rule.", + Description: "The primary data center in which to run the integration rule.",🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required -app_id(String) The Ably application ID. -target(Attributes) object (rul...(A_RB_NN)
internal/provider/resource_ably_ingress_rule_mongo.go (1)
91-94: Consider making import field names configurableThe
ImportResourcefunction uses hardcoded field names "app_id" and "id". Consider making these configurable to improve reusability.-ImportResource(ctx, req, resp, "app_id", "id") +func (r resourceIngressRuleMongo) ImportFields() (string, string) { + return "app_id", "id" +} + +ImportResource(ctx, req, resp, r.ImportFields())
🛑 Comments failed to post (5)
internal/provider/ingress_rules.go (2)
43-49: 🛠️ Refactor suggestion
Validate required fields before conversion
The function creates a new rule without validating that required fields are present and non-empty.
Add validation before creating the rule:
+ if plan.Status.IsNull() || plan.Status.ValueString() == "" { + panic("Status is required") + } + if target == nil { + panic("Target is required and must be a supported type") + } rule_values := ably_control_go.NewIngressRule{ Status: plan.Status.ValueString(), Target: target, }📝 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.if plan.Status.IsNull() || plan.Status.ValueString() == "" { panic("Status is required") } if target == nil { panic("Target is required and must be a supported type") } rule_values := ably_control_go.NewIngressRule{ Status: plan.Status.ValueString(), Target: target, } return rule_values }
110-114: 🛠️ Refactor suggestion
Add validation for status field values
The status field should be validated against allowed values.
Add validation for the status field:
"status": { Type: types.StringType, Optional: true, Description: "The status of the rule. Rules can be enabled or disabled.", + Validators: []tfsdk.AttributeValidator{ + stringvalidator.OneOf("enabled", "disabled"), + }, },📝 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."status": { Type: types.StringType, Optional: true, Description: "The status of the rule. Rules can be enabled or disabled.", Validators: []tfsdk.AttributeValidator{ stringvalidator.OneOf("enabled", "disabled"), }, },docs/resources/ingress_rule_postgres_outbox.md (1)
6-6: 🛠️ Refactor suggestion
Fix grammar and remove redundant description
The description has two issues:
- Grammar: "resource Use" should be "resource Uses"
- The description is duplicated in lines 6 and 11
Apply this diff to fix the issues:
- The ably_ingress_rule_postgres_outbox resource Use the Postgres database connector + The ably_ingress_rule_postgres_outbox resource Uses the Postgres database connectorAnd remove the redundant description in line 11.
Also applies to: 11-11
internal/provider/resource_ably_ingress_rule_postgres_outbox.go (1)
67-67: 🛠️ Refactor suggestion
Fix grammar in resource description
The description has the same grammar issue as in the documentation.
Apply this diff to fix the grammar:
- "The `ably_ingress_rule_postgres_outbox` resource Use the Postgres database connector + "The `ably_ingress_rule_postgres_outbox` resource Uses the Postgres database connector📝 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."The `ably_ingress_rule_postgres_outbox` resource Uses the Postgres database connector to distribute changes from your Postgres database to end users at scale. It enables you to distribute records using the outbox pattern to large numbers of subscribing clients, in realtime, as the changes occur."), nilinternal/provider/resource_ably_ingress_rule_mongo.go (1)
1-1:
⚠️ Potential issuePackage name should match directory structure
The package name
ably_controldoesn't match the directory structureinternal/provider/. Consider renaming it toproviderto maintain consistency with the directory structure.-package ably_control +package provider📝 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.package provider
This adds the new Ably Ingress rule for MongoDB.
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
from
a3005b9 to
c86aac0
Compare
This adds the new Ably Ingress rule for Postgres Outbox. Documentation on this rule can be found at https://ably.com/docs/livesync/postgres
graham-russell
force-pushed
the
feature/INF-5307-add-ingress-rules
branch
from
c86aac0 to
832f77f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
internal/provider/ingress_rules.go (2)
19-41:⚠️ Potential issueAdd a default case to handle unrecognized target types
This switch statement only handles MongoDB and PostgreSQL. If additional target types are introduced or an unexpected type is passed in, the code will silently do nothing, potentially leading to nil pointer dereferences later. This was flagged in a previous review.Consider adding a default case:
switch t := plan.Target.(type) { case *AblyIngressRuleTargetMongo: // ... case *AblyIngressRuleTargetPostgresOutbox: // ... +default: + panic(fmt.Sprintf("Unsupported target type: %T", t)) }
56-78:⚠️ Potential issueAdd a default case to handle unrecognized target types in GetIngressRuleResponse
Similarly, this switch statement only handles MongoDB and PostgreSQL. An unexpected or missing type could result in a nil target or silent failures. This was also flagged previously.Consider adding a default case:
switch v := ably_ingress_rule.Target.(type) { case *ably_control_go.IngressMongoTarget: // ... case *ably_control_go.IngressPostgresOutboxTarget: // ... +default: + panic(fmt.Sprintf("Unsupported target type in response: %T", v)) }internal/provider/resource_ably_ingress_rule_mongo.go (1)
16-57: 🛠️ Refactor suggestionConsider optional validators
While each field has a descriptive string, the code lacks validators (e.g., ensuring the pipeline field is valid JSON). Adding them could prevent invalid configurations from reaching the SDK. This was previously suggested and remains valuable.
🧹 Nitpick comments (7)
internal/provider/ingress_rules.go (2)
140-153: Confirm that plan_values are validated
When creating a new rule, the code retrieves plan values and immediately invokes the SDK. If any validation errors exist (e.g., invalid URL/SSL parameters), they won’t be flagged until the provider library call. Consider adding more robust validation either here or in the schema validation logic to catch issues earlier.
295-318: Import usage documentation
The import block relies on a comma-separated string to parse the composite ID. Consider adding user-facing documentation (e.g., in the resource docs) on how to perform the import. This fosters clarity on the format "app_id,rule_id".docs/resources/ingress_rule_mongodb.md (2)
1-7: Expand usage examples
While this documentation is good, consider adding a short usage example or a sample code snippet to help users quickly get started.
20-22: Ensure clarity on “Ably application ID”
The phrase “The Ably application ID” is acceptable, but consider rephrasing lightly for consistency across all docs (e.g., “Enter your Ably application ID”).🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required -app_id(String) The Ably application ID. -target(Attributes) object (rul...(A_RB_NN)
docs/resources/ingress_rule_postgres_outbox.md (2)
1-7: Refine resource description
The resource description might read more naturally by replacing “Use the Postgres database connector…” with “This resource uses the Postgres database connector…”.
40-47: Clarify default SSL behavior
You have added a note that the default value for ssl_mode is "prefer". It might help users to see that it attempts an SSL connection but does not fail on fallback.internal/provider/resource_ably_ingress_rule_mongo.go (1)
56-56: Confirm reference link or extra documentation
You are providing a description that references how to set up the resource, but you might also include a direct link to relevant Ably docs if available. This approach helps users more quickly locate advanced usage examples or configuration tips.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
.github/workflows/check.yml(1 hunks)COPYRIGHT(1 hunks)Makefile(2 hunks)docs/resources/ingress_rule_mongodb.md(1 hunks)docs/resources/ingress_rule_postgres_outbox.md(1 hunks)docs/resources/rule_azure_function.md(1 hunks)examples/playground/ingress_rule_mongodb.tf(1 hunks)examples/playground/ingress_rule_postgres_outbox.tf(1 hunks)examples/playground/main.tf(1 hunks)examples/playground/rule_http_azure_function.tf(1 hunks)examples/playground/variables.tf(1 hunks)examples/resources/rule_http_azure_function.tf(1 hunks)go.mod(1 hunks)internal/provider/ingress_rules.go(1 hunks)internal/provider/models.go(1 hunks)internal/provider/provider.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo.go(1 hunks)internal/provider/resource_ably_ingress_rule_mongo_test.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox.go(1 hunks)internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go(1 hunks)internal/provider/resource_ably_namespace_test.go(2 hunks)internal/provider/resource_ably_rule_azure_function_test.go(2 hunks)templates/index.md.tmpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- COPYRIGHT
- docs/resources/rule_azure_function.md
- examples/playground/rule_http_azure_function.tf
- .github/workflows/check.yml
- examples/playground/main.tf
- internal/provider/resource_ably_namespace_test.go
- go.mod
- examples/resources/rule_http_azure_function.tf
- templates/index.md.tmpl
- internal/provider/resource_ably_rule_azure_function_test.go
- examples/playground/ingress_rule_postgres_outbox.tf
- examples/playground/ingress_rule_mongodb.tf
- internal/provider/provider.go
- internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go
- internal/provider/models.go
- examples/playground/variables.tf
- internal/provider/resource_ably_ingress_rule_mongo_test.go
🧰 Additional context used
🪛 LanguageTool
docs/resources/ingress_rule_mongodb.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
docs/resources/ingress_rule_postgres_outbox.md
[grammar] ~20-~20: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘application’) instead of another adjective.
Context: ... ### Required - app_id (String) The Ably application ID. - target (Attributes) object (rul...
(A_RB_NN)
🔇 Additional comments (6)
Makefile (4)
6-6: LGTM: Version update aligns with project versioning.
The version bump to 0.7.0 is appropriate and consistent with the project's version history.
7-14: LGTM: Robust architecture detection system.
The implementation includes proper OS detection, architecture mapping, and validation against supported architectures. The validation logic correctly checks if the mapped architecture is one of the supported types (amd64, arm64, arm).
23-23: LGTM: Complete arm64 architecture support.
The addition of arm64 build targets for both Darwin and Linux platforms completes the architecture support matrix and aligns with the architecture detection system.
Also applies to: 29-29
14-14: Verify installation directory permissions.
While the dynamic OS_ARCH path is correct, ensure that the installation directory has appropriate permissions.
Also applies to: 38-40
✅ Verification successful
Let me check the Makefile to see how the directory creation and permissions are handled.
Installation directory permissions are handled correctly
The Makefile uses mkdir -p which creates the directory with default permissions (typically 755 for directories), and this is the standard approach for Terraform plugin installations. The permissions will be inherited from the user's umask, which is appropriate for Terraform plugins.
- The directory is created under the user's home directory (
~/.terraform.d/plugins/) - The directory structure follows Terraform's plugin directory convention
- Default permissions are sufficient as this is a user-specific installation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the terraform plugins directory exists and has correct permissions
# Test: Check if the directory exists and its permissions
echo "Checking Terraform plugins directory..."
PLUGIN_DIR="$HOME/.terraform.d/plugins"
if [ -d "$PLUGIN_DIR" ]; then
ls -la "$PLUGIN_DIR"
echo "Directory exists with permissions: $(stat -f %A "$PLUGIN_DIR")"
else
echo "Directory does not exist. It will be created with default permissions."
fi
Length of output: 337
Script:
#!/bin/bash
# Check Makefile for directory creation and permission handling
rg -A 3 "mkdir|install" Makefile
Length of output: 287
internal/provider/resource_ably_ingress_rule_postgres_outbox.go (2)
12-14: LGTM!
The resource struct definition is clean and follows Terraform provider conventions.
82-100: Verify generic CRUD implementation
The CRUD operations use generic functions with AblyIngressRuleTargetPostgresOutbox. Let's verify this type and its implementation.
✅ Verification successful
Let me verify the generic CRUD implementation by checking the generic functions being used. I'll generate another script to examine these functions.
Generic CRUD implementation is correctly implemented
The verification confirms that:
- The
AblyIngressRuleTargetPostgresOutboxstruct is properly defined with all required fields - The generic CRUD functions (
CreateIngressRule,ReadIngressRule,UpdateIngressRule,DeleteIngressRule) are properly implemented with type parameters - The functions handle proper error cases, state management, and API client interactions
- The type is correctly used in the resource implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the generic type implementation and its usage
# Check for the type definition
ast-grep --pattern 'type AblyIngressRuleTargetPostgresOutbox struct {
$$$
}'
# Check for other usages of this type
rg "AblyIngressRuleTargetPostgresOutbox" -A 5
Length of output: 4675
Script:
#!/bin/bash
# Check the generic CRUD functions implementation
ast-grep --pattern 'func CreateIngressRule[$_]($_, $_, $_, $_) {
$$$
}'
# Check other generic functions
ast-grep --pattern 'func ReadIngressRule[$_]($_, $_, $_, $_) {
$$$
}'
ast-grep --pattern 'func UpdateIngressRule[$_]($_, $_, $_, $_) {
$$$
}'
ast-grep --pattern 'func DeleteIngressRule[$_]($_, $_, $_, $_) {
$$$
}'
Length of output: 11104
Updated to use the recently released version of ably-control-go (v.5.0)
**Implemented enhancements:** - Expose LiveSync to the terraform provider [\#180](#180) **Closed issues:** - Following instructions for importing configuration doesn't work [\#181](#181) **Merged pull requests:** - Bump golang.org/x/crypto from 0.21.0 to 0.31.0 [\#188](#188) ([dependabot[bot]](https://github.com/apps/dependabot)) - \[INF-5307\] - Add the MongoDB & PostgreSQL Outbox Ably Ingress Rules [\#187](#187) ([graham-russell](https://github.com/graham-russell)) - Update documentation for importing existing apps to use app id instead of a name [\#186](#186) ([kavalerov](https://github.com/kavalerov)) - Update goreleaser [\#185](#185) ([surminus](https://github.com/surminus))
This adds the new Ably Ingress rule for MongoDB.
Summary by CodeRabbit
New Features
ably_ingress_rule_mongodbresource.ably_ingress_rule_postgres_outboxresource.Bug Fixes
azure_app_idfield in multiple resources for consistency.Documentation
Tests
ably_ingress_rule_mongodbandably_ingress_rule_postgres_outboxresources.ably_rule_azure_functionresource.Chores