Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(logs): add QueryDefinition L2 Construct #18655

Merged
merged 10 commits into from
Apr 20, 2022
Merged

feat(logs): add QueryDefinition L2 Construct #18655

merged 10 commits into from
Apr 20, 2022

Conversation

jerry-shao
Copy link
Member

This PR implemented the [feature request] (aws-cloudwatch): Saved queries. It will let users be able to create CloudWatch Logs Insights QueryDefinition by using L2 construct.

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

@gitpod-io
Copy link

gitpod-io bot commented Jan 25, 2022

@github-actions github-actions bot added the @aws-cdk/aws-logs Related to Amazon CloudWatch Logs label Jan 25, 2022
@jerry-shao jerry-shao mentioned this pull request Jan 25, 2022
Closed
2 tasks
@jerry-shao
Copy link
Member Author

Hi @comcalvi and @madeline-k

Any suggested reviewer for this PR?

@comcalvi
Copy link
Contributor

@jerry-shao thanks for the PR, I will review this no later than next week.

comcalvi
comcalvi previously requested changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! It needs some work to further differentiate it from the L1, but this is a great start.

packages/@aws-cdk/aws-logs/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/test/integ.save-logs-insights-query-definition.expected.json Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/test/integ.save-logs-insights-query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/test/query-definition.test.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed comcalvi’s stale review March 16, 2022 15:09

Pull request has been modified.

@jerry-shao jerry-shao requested a review from comcalvi March 16, 2022 16:41
@jerry-shao
Copy link
Member Author

Hi @comcalvi

I have added a QueryString object with some methods to add different commands.

Please take a look, thanks!

comcalvi
comcalvi previously requested changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! I'd like to see a few quality of life changes and more uses of our existing constructs instead of raw string values, but this is coming along nicely. Nice work!

packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
@jerry-shao jerry-shao force-pushed the jerry-shao/logs-query-definition branch from 72561b0 to e2333ab Compare April 18, 2022 11:03
@mergify mergify bot dismissed comcalvi’s stale review April 18, 2022 11:03

Pull request has been modified.

@jerry-shao jerry-shao requested a review from comcalvi April 18, 2022 11:35
@jerry-shao
Copy link
Member Author

Hi @comcalvi

Thanks for your comments, I have addressed them in the newer commit, please take a look.

Thanks!

comcalvi
comcalvi previously requested changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! A few small comments and this should be ready to merge.

packages/@aws-cdk/aws-logs/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/query-definition.ts Outdated Show resolved Hide resolved
@jerry-shao jerry-shao force-pushed the jerry-shao/logs-query-definition branch from 0aba264 to 03fcd99 Compare April 19, 2022 22:58
@mergify mergify bot dismissed comcalvi’s stale review April 19, 2022 22:58

Pull request has been modified.

@jerry-shao
Copy link
Member Author

@comcalvi Thanks for your comments.

I have addressed all of them, the CodeBuild should pass now :D

@jerry-shao jerry-shao requested a review from comcalvi April 19, 2022 22:59
@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2022

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 4e28c56
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit fcf981b into aws:master Apr 20, 2022
@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2022

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

@jerry-shao jerry-shao deleted the jerry-shao/logs-query-definition branch April 22, 2022 19:46
StevePotter pushed a commit to StevePotter/aws-cdk that referenced this pull request Apr 27, 2022
@jerry-shao
feat(logs): add QueryDefinition L2 Construct (aws#18655)
ee237ad 
This PR implemented the [[feature request] (aws-cloudwatch): Saved queries](aws#16395). It will let users be able to create CloudWatch Logs Insights QueryDefinition by using L2 construct.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comcalvi comcalvi comcalvi approved these changes

Assignees

@comcalvi comcalvi

Labels
@aws-cdk/aws-logs Related to Amazon CloudWatch Logs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jerry-shao @comcalvi @aws-cdk-automation