feat(scheduler-targets): SqsSendMessage Target #27774

go-to-k · 2023-10-31T12:42:03Z

This PR adds SqsSendMessage Target for EventBridge Scheduler.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

go-to-k · 2023-11-01T03:59:55Z

packages/@aws-cdk/aws-scheduler-targets-alpha/lib/sqs-send-message.ts

+      if (props.messageGroupId.length < 1 || props.messageGroupId.length > 128 ) {
+        throw new Error(`messageGroupId length must be between 1 and 128, got ${props.messageGroupId.length}`);
+      }


The doc is here.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-scheduler-schedule-sqsparameters.html#cfn-scheduler-schedule-sqsparameters-messagegroupid

go-to-k · 2023-11-01T04:00:31Z

packages/@aws-cdk/aws-scheduler-targets-alpha/lib/sqs-send-message.ts

+    } else if (queue.fifo) {
+      throw new Error('messageGroupId must be specified if the target is a FIFO queue');
+    }


Contains the message group ID to use when the target is a FIFO queue.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-scheduler-schedule-target.html#cfn-scheduler-schedule-target-sqsparameters

go-to-k · 2023-11-01T04:00:57Z

packages/@aws-cdk/aws-scheduler-targets-alpha/lib/sqs-send-message.ts

+      if (!(queue.node.defaultChild as sqs.CfnQueue).contentBasedDeduplication) {
+        throw new Error('contentBasedDeduplication must be true if the target is a FIFO queue');
+      }


If you specify an Amazon SQS FIFO queue as a target, the queue must have content-based deduplication enabled.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-scheduler-schedule-target.html#cfn-scheduler-schedule-target-sqsparameters

go-to-k · 2023-11-01T04:02:40Z

packages/@aws-cdk/aws-scheduler-targets-alpha/lib/sqs-send-message.ts

+      if (!queue.fifo) {
+        throw new Error('target must be a FIFO queue if messageGroupId is specified');
+      }


CFn occurs a following error when the queue is not FIFO queue if messageGroupId is specified. So added this validation.

ROLLBACK_COMPLETE: Resource handler returned message: "Invalid request provided: Parameters MessageGroupId not valid for the target.

go-to-k · 2023-11-02T11:20:27Z

@kaizencc

For some reason this PR doesn't seem to have a pr/needs-community-review label. How can I label it? Did this happen because I already commented on it?

laurelmay

The links to documentation are super helpful! Thanks! The tests also seem to cover the cases well.

I do have two small thoughts I wanted to ask about though.

laurelmay · 2023-11-03T05:20:39Z

packages/@aws-cdk/aws-scheduler-targets-alpha/lib/sqs-send-message.ts

+      throw new Error(`Cannot grant permission to execution role in account ${this.props.role.env.account} to invoke target ${Names.nodeUniqueId(this.queue.node)} in account ${this.queue.env.account}. Both the target and the execution role must be in the same account.`);
+    }
+
+    this.queue.grant(role, 'sqs:SendMessage');


Suggested change

this.queue.grant(role, 'sqs:SendMessage');

this.queue.grantSendMessages(role);

Would this be better? It handles the case where the queue uses KMS encryption and grants a few other permissions. But I'm not sure that's necessary for this case?

Certainly, I'll leave it at that.

laurelmay · 2023-11-03T05:28:41Z

packages/@aws-cdk/aws-scheduler-targets-alpha/lib/sqs-send-message.ts

+      if (props.messageGroupId.length < 1 || props.messageGroupId.length > 128 ) {
+        throw new Error(`messageGroupId length must be between 1 and 128, got ${props.messageGroupId.length}`);
+      }


I don't think tokens ever get this long but maybe:

Suggested change

if (props.messageGroupId.length < 1 || props.messageGroupId.length > 128 ) {

throw new Error(`messageGroupId length must be between 1 and 128, got ${props.messageGroupId.length}`);

}

if (!Token.isUnresolved(props.messageGroupId) && (props.messageGroupId.length < 1 || props.messageGroupId.length > 128)) {

throw new Error(`messageGroupId length must be between 1 and 128, got ${props.messageGroupId.length}`);

}

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.Token.html#static-iswbrunresolvedobj

Yes, I'll do that just in case.

go-to-k · 2023-11-04T09:23:51Z

@kylelaker

Thanks for your review!

I changed, so please review again.

laurelmay

Thank you!

…cheduler-targets-sqs

sumupitchayan

Thanks for contributing!

mergify · 2023-11-20T20:28:40Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2023-11-20T20:32:24Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2023-11-21T14:48:44Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-11-21T15:31:47Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 84951af
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-11-21T15:32:31Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

go-to-k added 6 commits October 31, 2023 20:21

wip

269b0bd

test: change unit tests

e397c2c

feat: add bindBaseTargetConfig

41fbf8b

docs: props comments

48dca93

test: change an unit test

32452a6

test: wip

ec9a162

aws-cdk-automation requested a review from a team October 31, 2023 12:42

github-actions bot added star-contributor [Pilot] contributed between 25-49 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Oct 31, 2023

aws-cdk-automation previously requested changes Oct 31, 2023

View reviewed changes

go-to-k added 3 commits October 31, 2023 23:29

test: add integ tests

e1b18f3

add validations and the unit tests

d1ab879

docs: WIP in README

38c443f

go-to-k added 2 commits November 1, 2023 12:58

chore: change validations and unit tests

173f2ed

docs: wip in README

13489af

go-to-k commented Nov 1, 2023

View reviewed changes

docs: change README and jsdocs

4fc9c97

go-to-k marked this pull request as ready for review November 1, 2023 04:31

go-to-k added 2 commits November 1, 2023 14:30

docs: fix for rosetta in README

5b2ed77

docs: add default for messageGroupId

97acac6

kaizencc changed the title ~~feat(scheduler-targets): Add SqsSendMessage Target~~ feat(scheduler-targets): SqsSendMessage Target Nov 1, 2023

laurelmay reviewed Nov 3, 2023

View reviewed changes

go-to-k added 2 commits November 4, 2023 18:04

Merge branch 'main' into feat/scheduler-targets-sqs

736e359

covered for the review

3bf56da

add a new line in README

c585d6c

laurelmay approved these changes Nov 4, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 4, 2023

go-to-k added 2 commits November 12, 2023 23:46

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into feat/s…

276298a

…cheduler-targets-sqs

improve unit tests by using fromQueueArn in the same stack

3060470

sumupitchayan previously approved these changes Nov 20, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 20, 2023

Merge branch 'main' into feat/scheduler-targets-sqs

bc9bddc

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 20, 2023

sumupitchayan previously approved these changes Nov 20, 2023

View reviewed changes

Merge branch 'main' into feat/scheduler-targets-sqs

84e77f6

go-to-k requested a review from sumupitchayan November 21, 2023 04:55

Merge branch 'main' into feat/scheduler-targets-sqs

c5bb455

sumupitchayan approved these changes Nov 21, 2023

View reviewed changes

mergify bot added 2 commits November 21, 2023 14:48

Merge branch 'main' into feat/scheduler-targets-sqs

6ad3e0c

Merge branch 'main' into feat/scheduler-targets-sqs

84951af

mergify bot merged commit 80c1d26 into aws:main Nov 21, 2023
9 checks passed

go-to-k deleted the feat/scheduler-targets-sqs branch November 21, 2023 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(scheduler-targets): SqsSendMessage Target #27774

feat(scheduler-targets): SqsSendMessage Target #27774

go-to-k commented Oct 31, 2023 •

edited

Loading

aws-cdk-automation left a comment

go-to-k Nov 1, 2023

go-to-k Nov 1, 2023

go-to-k Nov 1, 2023

go-to-k Nov 1, 2023

go-to-k commented Nov 2, 2023 •

edited

Loading

laurelmay left a comment

laurelmay Nov 3, 2023

go-to-k Nov 4, 2023

laurelmay Nov 3, 2023 •

edited

Loading

go-to-k Nov 4, 2023

go-to-k commented Nov 4, 2023

laurelmay left a comment

sumupitchayan left a comment

mergify bot commented Nov 20, 2023

mergify bot commented Nov 20, 2023

mergify bot commented Nov 21, 2023

aws-cdk-automation commented Nov 21, 2023

mergify bot commented Nov 21, 2023

	this.queue.grant(role, 'sqs:SendMessage');
	this.queue.grantSendMessages(role);

feat(scheduler-targets): SqsSendMessage Target #27774

feat(scheduler-targets): SqsSendMessage Target #27774

Conversation

go-to-k commented Oct 31, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

go-to-k Nov 1, 2023

Choose a reason for hiding this comment

go-to-k Nov 1, 2023

Choose a reason for hiding this comment

go-to-k Nov 1, 2023

Choose a reason for hiding this comment

go-to-k Nov 1, 2023

Choose a reason for hiding this comment

go-to-k commented Nov 2, 2023 • edited Loading

laurelmay left a comment

Choose a reason for hiding this comment

laurelmay Nov 3, 2023

Choose a reason for hiding this comment

go-to-k Nov 4, 2023

Choose a reason for hiding this comment

laurelmay Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

go-to-k Nov 4, 2023

Choose a reason for hiding this comment

go-to-k commented Nov 4, 2023

laurelmay left a comment

Choose a reason for hiding this comment

sumupitchayan left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 20, 2023

mergify bot commented Nov 20, 2023

mergify bot commented Nov 21, 2023

aws-cdk-automation commented Nov 21, 2023

AWS CodeBuild CI Report

mergify bot commented Nov 21, 2023

go-to-k commented Oct 31, 2023 •

edited

Loading

go-to-k commented Nov 2, 2023 •

edited

Loading

laurelmay Nov 3, 2023 •

edited

Loading