-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
base: main
Are you sure you want to change the base?
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
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Add CloudWatch Logs as a Pipe target.