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.

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(logs): specify log group's region for LogRetention #9804

Merged
merged 8 commits into from
Sep 2, 2020

Conversation

Stacy-D
Copy link
Contributor

@Stacy-D Stacy-D commented Aug 18, 2020

Global AWS services follow the convention of auto-creating CloudWatch Logs in us-east-1, which
makes it challenging to control the log group retention when the stack is created in other region.

This change adds support for specifying the optional region for LogRetention to be able to set log retention on CloudWatch logs created by global AWS Services in us-east-1

Closes #9703


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

@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@Stacy-D Stacy-D changed the title feat(aws-lambda): add support for setting the region in LogRetention #9703 feat(aws-lambda): add support for setting the region in LogRetention Aug 18, 2020
Copy link
Contributor

@nija-at nija-at 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 submitting this PR 😊

I would like this one to wait until this PR is merged - #9808.

@Stacy-D
Copy link
Contributor Author

Stacy-D commented Aug 19, 2020

That's fine to wait for #9808. We were doing these changes(move and adding the region) in parallel so they resulted in two dependent PRs. I believe the changes from my side after #9808 should involve moving this code to the LogRetention in aws-logs and addressing any other comments that you might have.

@humanzz
Copy link
Contributor

humanzz commented Aug 26, 2020

Hey @Stacy-D, #9808 has now been merged and you can now progress this :)

Global AWS services follow the convention of autocreating CloudWatch Logs in us-east-1, which
makes it challenging to control the log group retention when the stack is created in other region.

This change adds support for specifying the optional region for LogRetention
@Stacy-D Stacy-D changed the title feat(aws-lambda): add support for setting the region in LogRetention feat(logs): add support for specifying the region in LogRetention Aug 27, 2020
@Stacy-D Stacy-D requested a review from nija-at August 27, 2020 14:22
@humanzz
Copy link
Contributor

humanzz commented Sep 1, 2020

Hey @nija-at, I think this is now ready for further review after rebasing on my changes.
Appreciate you having a look so we can move forwards with this.

nija-at
nija-at previously requested changes Sep 1, 2020
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Just a couple of comments around documentation.

@@ -32,6 +32,13 @@ retention period (including infinite retention).

[retention example](test/example.retention.lit.ts)

**Note** When using `LogRetention` to configure retention period for the log group,
Copy link
Contributor

Choose a reason for hiding this comment

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

This was missed in the previous PR - can we add a separate section in the README dedicated to the LogRetention construct?

I suspect it will look something similar to - https://github.com/aws/aws-cdk/tree/master/packages/@aws-cdk/aws-lambda#singleton-function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new entry

packages/@aws-cdk/aws-logs/lib/log-retention.ts Outdated Show resolved Hide resolved
@nija-at nija-at changed the title feat(logs): add support for specifying the region in LogRetention feat(logs): specify log group's region for LogRetention Sep 1, 2020
@mergify mergify bot dismissed nija-at’s stale review September 1, 2020 15:57

Pull request has been modified.

Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

I've taken the liberty to re-write some sections in the README.

packages/@aws-cdk/aws-logs/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/README.md Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2020

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: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 4d11555
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2020

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[lambda][logs] Make LogRetention construct support global AWS Services that write logs to us-east-1
4 participants