-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(pipes-targets): add CloudWatch Logs #30665
Conversation
a90f01f
to
a6f3013
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm no regex expert but I can't make sense of that 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@redwheeler3 is there a disconnect between the docs and the API? or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
packages/@aws-cdk/aws-pipes-targets-alpha/test/integ.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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, 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.