Merged
Conversation
edgarfgp
force-pushed
the
fix-sqs-builder
branch
from
8ebbb77 to
2aece60
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request significantly enhances the SQS queue builder DSL by renaming operations for clarity and adding support for many additional AWS SQS queue properties that were previously unavailable.
Key Changes
- Renamed
messageRetentiontoretentionPeriodanddelaySecondstodeliveryDelayfor better API consistency - Added support for 9 new SQS properties:
DeduplicationScope,EnforceSSL,FifoThroughputLimit,MaxMessageSizeBytes,ReceiveMessageWaitTime,RedriveAllowPolicy,RemovalPolicy,DataKeyReuse, andDeadLetterQueue - Standardized Duration-based operations to accept
Durationobjects instead of primitive numeric types for better type safety
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SQS.fs | Core implementation: expanded QueueConfig record with new fields, renamed operations (messageRetention → retentionPeriod, delaySeconds → deliveryDelay, FifoQueue → Fifo), added 9 new custom operations with full XML documentation |
| tests/SQSTests.fs | New comprehensive test file covering FIFO configuration, encryption, deduplication scope, throughput limits, SSL enforcement, and message size settings |
| tests/FsCdk.Tests.fsproj | Added SQSTests.fs to the test project compilation order |
| tests/AppTests.fs | Updated usage example from messageRetention to retentionPeriod |
| docs/sns-sqs-messaging.fsx | Updated all queue examples to use retentionPeriod instead of messageRetention |
| docs/lambda-production-defaults.fsx | Updated custom DLQ example to use retentionPeriod |
| docs/index.fsx | Updated queue examples to use retentionPeriod; documentation table still references delaySeconds instead of deliveryDelay |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+151
| module FsCDK.Tests.SQSTests | ||
|
|
||
| open Expecto | ||
| open FsCDK | ||
| open Amazon.CDK | ||
| open Amazon.CDK.AWS.SQS | ||
|
|
||
| [<Tests>] | ||
| let sqs_queue_dsl_tests = | ||
| testList | ||
| "SQS queue DSL" | ||
| [ test "defaults constructId to queue name" { | ||
| let spec = queue "MyQueue" { () } | ||
|
|
||
| Expect.equal spec.QueueName "MyQueue" "QueueName should be set" | ||
| Expect.equal spec.ConstructId "MyQueue" "ConstructId should default to queue name" | ||
| } | ||
|
|
||
| test "uses custom constructId when provided" { | ||
| let spec = queue "MyQueue" { constructId "MyQueueConstruct" } | ||
|
|
||
| Expect.equal spec.ConstructId "MyQueueConstruct" "Custom constructId should be used" | ||
| } | ||
|
|
||
| test "configures FIFO queue" { | ||
| let spec = queue "MyQueue.fifo" { fifo true } | ||
|
|
||
| Expect.isTrue spec.Props.Fifo.Value "Queue should be configured as FIFO" | ||
| } | ||
|
|
||
| test "enables content-based deduplication" { | ||
| let spec = | ||
| queue "MyQueue.fifo" { | ||
| fifo true | ||
| contentBasedDeduplication true | ||
| } | ||
|
|
||
| Expect.isTrue spec.Props.ContentBasedDeduplication.Value "ContentBasedDeduplication should be enabled" | ||
| } | ||
|
|
||
| test "configures dead-letter queue reference" { | ||
| let spec = queue "MyQueue" { deadLetterQueue "MyDLQ" 3 } | ||
|
|
||
| // Note: The actual DLQ connection happens at stack build time | ||
| Expect.equal spec.QueueName "MyQueue" "Queue name should be set" | ||
| } | ||
|
|
||
| test "applies encryption when configured" { | ||
| let spec = queue "MyQueue" { encryption QueueEncryption.KMS_MANAGED } | ||
|
|
||
| Expect.equal spec.Props.Encryption.Value QueueEncryption.KMS_MANAGED "Encryption should be set" | ||
| } | ||
|
|
||
| test "applies deduplication scope for FIFO queues" { | ||
| let spec = | ||
| queue "MyQueue.fifo" { | ||
| fifo true | ||
| deduplicationScope DeduplicationScope.MESSAGE_GROUP | ||
| } | ||
|
|
||
| Expect.equal | ||
| spec.Props.DeduplicationScope.Value | ||
| DeduplicationScope.MESSAGE_GROUP | ||
| "DeduplicationScope should be set" | ||
| } | ||
|
|
||
| test "enforces SSL when configured" { | ||
| let spec = queue "MyQueue" { enforceSSL true } | ||
|
|
||
| Expect.isTrue spec.Props.EnforceSSL.Value "EnforceSSL should be true" | ||
| } | ||
|
|
||
| test "applies FIFO throughput limit" { | ||
| let spec = | ||
| queue "MyQueue.fifo" { | ||
| fifo true | ||
| fifoThroughputLimit FifoThroughputLimit.PER_MESSAGE_GROUP_ID | ||
| } | ||
|
|
||
| Expect.equal | ||
| spec.Props.FifoThroughputLimit.Value | ||
| FifoThroughputLimit.PER_MESSAGE_GROUP_ID | ||
| "FifoThroughputLimit should be set" | ||
| } | ||
|
|
||
| test "applies max message size bytes" { | ||
| let spec = queue "MyQueue" { maxMessageSizeBytes 65536.0 } | ||
|
|
||
| Expect.equal spec.Props.MaxMessageSizeBytes.Value 65536.0 "MaxMessageSizeBytes should be set" | ||
| } | ||
|
|
||
| test "applies removal policy" { | ||
| let spec = queue "MyQueue" { removalPolicy RemovalPolicy.DESTROY } | ||
|
|
||
| Expect.equal spec.Props.RemovalPolicy.Value RemovalPolicy.DESTROY "RemovalPolicy should be set" | ||
| } | ||
|
|
||
| test "combines multiple non-Duration configurations" { | ||
| let spec = | ||
| queue "MyQueue.fifo" { | ||
| constructId "MyFifoQueue" | ||
| fifo true | ||
| contentBasedDeduplication true | ||
| deduplicationScope DeduplicationScope.MESSAGE_GROUP | ||
| fifoThroughputLimit FifoThroughputLimit.PER_MESSAGE_GROUP_ID | ||
| enforceSSL true | ||
| maxMessageSizeBytes 131072.0 | ||
| removalPolicy RemovalPolicy.RETAIN | ||
| } | ||
|
|
||
| Expect.equal spec.ConstructId "MyFifoQueue" "ConstructId should be set" | ||
| Expect.isTrue spec.Props.Fifo.Value "Fifo should be true" | ||
| Expect.isTrue spec.Props.ContentBasedDeduplication.Value "ContentBasedDeduplication should be true" | ||
|
|
||
| Expect.equal | ||
| spec.Props.DeduplicationScope.Value | ||
| DeduplicationScope.MESSAGE_GROUP | ||
| "DeduplicationScope should be set" | ||
|
|
||
| Expect.equal | ||
| spec.Props.FifoThroughputLimit.Value | ||
| FifoThroughputLimit.PER_MESSAGE_GROUP_ID | ||
| "FifoThroughputLimit should be set" | ||
|
|
||
| Expect.isTrue spec.Props.EnforceSSL.Value "EnforceSSL should be true" | ||
| Expect.equal spec.Props.MaxMessageSizeBytes.Value 131072.0 "MaxMessageSizeBytes should be set" | ||
| Expect.equal spec.Props.RemovalPolicy.Value RemovalPolicy.RETAIN "RemovalPolicy should be set" | ||
| } | ||
|
|
||
| test "optional settings remain unset when not provided" { | ||
| let spec = queue "SimpleQueue" { () } | ||
|
|
||
| Expect.isNull (box spec.Props.Fifo) "Fifo should be null when not configured" | ||
|
|
||
| Expect.isNull | ||
| (box spec.Props.ContentBasedDeduplication) | ||
| "ContentBasedDeduplication should be null when not configured" | ||
|
|
||
| Expect.isNull (box spec.Props.Encryption) "Encryption should be null when not configured" | ||
| Expect.isNull (box spec.Props.EnforceSSL) "EnforceSSL should be null when not configured" | ||
|
|
||
| Expect.isNull | ||
| (box spec.Props.FifoThroughputLimit) | ||
| "FifoThroughputLimit should be null when not configured" | ||
|
|
||
| Expect.isNull | ||
| (box spec.Props.MaxMessageSizeBytes) | ||
| "MaxMessageSizeBytes should be null when not configured" | ||
|
|
||
| Expect.isNull (box spec.Props.RemovalPolicy) "RemovalPolicy should be null when not configured" | ||
| } ] |
There was a problem hiding this comment.
The new test file lacks coverage for several important operations that were added or modified in the SQS builder. Missing test coverage for:
retentionPeriod(renamed frommessageRetention)visibilityTimeoutdeliveryDelay(renamed fromdelaySeconds)dataKeyReusereceiveMessageWaitTimeredriveAllowPolicy
Consider adding tests for these operations to ensure they correctly set the corresponding properties on QueueProps.
Collaborator
Author
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.