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

refactor(lambda): move log retention custom resource to logs (#9671) #9808

Merged
merged 15 commits into from
Aug 26, 2020
Merged

refactor(lambda): move log retention custom resource to logs (#9671) #9808

merged 15 commits into from
Aug 26, 2020

Conversation

humanzz
Copy link
Contributor

@humanzz humanzz commented Aug 18, 2020

move LogRetention construct definition from lambda to logs while refactoring it so it does not depend on lambda constructs
this required reimplementing the functionality provided by lambda.SingletonFunction using CfnResource

keep declared classes/interfaces in lambda for compatability while marking them as deprecated
they should be removed in an upcoming breaking change for their current customers in lambda and rds

remove rds dependency on lambda

Fixes #9671


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

@humanzz humanzz marked this pull request as ready for review August 18, 2020 19:36
@nija-at nija-at self-assigned this Aug 19, 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.

Nice! I like how this is coming out.

Just one comment about the logical ids below. It feels like we can go all the way and have no diffs in the generated stacks.

packages/@aws-cdk/aws-logs/lib/log-retention.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed nija-at’s stale review August 24, 2020 10:35

Pull request has been modified.

@humanzz humanzz marked this pull request as draft August 24, 2020 14:31
@humanzz
Copy link
Contributor Author

humanzz commented Aug 24, 2020

Apologies for the multiple noisy commits, my local environment is having issues and I'm using CodeBuild for build verification. Changed the PR to draft mode till I get a proper working version.

@humanzz humanzz marked this pull request as ready for review August 24, 2020 16:25
@humanzz humanzz requested a review from nija-at August 24, 2020 16:25
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.

Good work on getting the logical ids aligned. Just a couple of comments around union types and dependencies.

packages/@aws-cdk/aws-lambda/lib/function.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/test/test.log-retention.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-logs/lib/log-retention.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-rds/package.json Show resolved Hide resolved
humanzz and others added 12 commits August 25, 2020 12:06
move LogRetention construct definition from lambda to logs while refactoring it so it does not depend on lambda constructs
this required reimplementing the functionality provided by lambda.SingletonFunction using CfnResource

keep declared classes/interfaces in lambda for compatability while marking them as deprecated
they should be removed in an upcoming breaking change for their current customers in lambda and rds

Fixes #9671
@mergify mergify bot dismissed nija-at’s stale review August 25, 2020 11:07

Pull request has been modified.

nija-at
nija-at previously approved these changes Aug 25, 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.

All clear 🙌

@mergify
Copy link
Contributor

mergify bot commented Aug 25, 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).

@humanzz
Copy link
Contributor Author

humanzz commented Aug 25, 2020

Great... looking forwards to this and the other changes we have lined up.

@humanzz
Copy link
Contributor Author

humanzz commented Aug 25, 2020

I'm merging and resolving conflicts

@mergify mergify bot dismissed nija-at’s stale review August 25, 2020 14:48

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: bdcbac2
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@humanzz humanzz requested a review from nija-at August 25, 2020 15:27
@humanzz
Copy link
Contributor Author

humanzz commented Aug 26, 2020

@nija-at seems like automerge didn’t kick off for some reason. It’s not clear to why that didn’t happen so appreciate letting me know if any further changes are needed on my end

@nija-at
Copy link
Contributor

nija-at commented Aug 26, 2020

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Aug 26, 2020

Command refresh: success

@nija-at nija-at merged commit 1d99ad6 into aws:master Aug 26, 2020
@nija-at
Copy link
Contributor

nija-at commented Aug 26, 2020

@humanzz - not sure what's going on with mergify there. I've manually merged this PR.

@humanzz
Copy link
Contributor Author

humanzz commented Aug 26, 2020

@nija-at a little hiccup it seems but glad we got this merged. Thanks :-)

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 reusable for any log group not just Lambda's
3 participants