feat(pipes-targets): add CloudWatch Logs#30665
Conversation
a90f01f to
a6f3013
Compare
nmussy
left a comment
There was a problem hiding this comment.
LGTM, just a couple of nitpicks
| * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-pipes-pipe-pipetargetcloudwatchlogsparameters.html#cfn-pipes-pipe-pipetargetcloudwatchlogsparameters-timestamp | ||
| * @default - none | ||
| */ | ||
| readonly timestamp?: string; |
There was a problem hiding this comment.
Any reason not to have a number here? You can convert it back to a string in the constructor, and it reduces the chance that a user enters something like a ISO date
There was a problem hiding this comment.
I can't get this to work in the Console even:
[ECMA 262 regex "^\$(\.[\w/_-]+(\[(\d+|\*)\])*)*$" does not match input string "1719416220650"]
There was a problem hiding this comment.
I'm no regex expert but I can't make sense of that 🤔
There was a problem hiding this comment.
@redwheeler3 is there a disconnect between the docs and the API? or am I missing something?
There was a problem hiding this comment.
For this one, we expect a JSONPath expression that references the timestamp in your payload. If you try the string "$.data.timestamp" it will validate. Leaving it blank updates it to the current time. We didn't think there was a use case here for specifying an actual time value... you either are going to want the current time or a time extracted from the payload of the event.
There was a problem hiding this comment.
@redwheeler3 I updated the above and added an example. I also confirmed $.data.timestamp worked in the Console. We should update the API docs.
05c4cfd to
8194e1f
Compare
bc1228a to
b733d84
Compare
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
9731036 to
04369fc
Compare
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
0aa06cb to
b647ed3
Compare
|
@Leo10Gama Could we keep rolling on these ? 😄 |
005a566 to
c879abd
Compare
Leo10Gama
left a comment
There was a problem hiding this comment.
Thanks for another contribution to the pipes targets! Just a few minor requests from my end, but overall looks good
packages/@aws-cdk/aws-pipes-targets-alpha/lib/cloudwatch-logs.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-pipes-targets-alpha/lib/cloudwatch-logs.ts
Outdated
Show resolved
Hide resolved
c879abd to
f785509
Compare
Pull request has been modified.
|
@Leo10Gama Fixed! Thanks for the review. |
f785509 to
f84ce1c
Compare
Leo10Gama
left a comment
There was a problem hiding this comment.
Thanks for the changes! I'm still only in disagreement on the current phrasing of the timestamp parameter, since we don't want to give the impression that it supports a value that might be a number. My understanding of how it looks now, we should also aim to describe the value itself, as opposed to exclusively defining it as being referenced by another value, if that makes sense.
packages/@aws-cdk/aws-pipes-targets-alpha/lib/cloudwatch-logs.ts
Outdated
Show resolved
Hide resolved
1980dfa to
b7efdbc
Compare
Leo10Gama
left a comment
There was a problem hiding this comment.
LGTM, thanks again for the contribution!
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
Add CloudWatch Logs as a Pipe target.