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): import a LogGroup from its name #5580

Merged
merged 4 commits into from
Dec 31, 2019
Merged

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Dec 30, 2019


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

@nija-at nija-at added the contribution/core This is a PR that came from AWS. label Dec 30, 2019
@nija-at nija-at requested a review from eladb December 30, 2019 11:13
@nija-at nija-at self-assigned this Dec 30, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

* Import an existing LogGroup given its name
*/
public static fromLogGroupName(scope: Construct, id: string, logGroupName: string): ILogGroup {
class Import extends LogGroupBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we reuse fromLogGroupArn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is that? Can you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just less duplication (not that there is a lot, but still).

Copy link
Contributor

Choose a reason for hiding this comment

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

public static fromLogGroupName(scope: Construct, id: string, logGroupName: string): ILogGroup {
  return this.fromLogGroupArn(scope, id, Stack.of(scope).formatArn({
    service: 'logs', resource: 'log-group', sep: ':', resourceName: logGroupName
  });
}

Copy link
Contributor Author

@nija-at nija-at Dec 30, 2019

Choose a reason for hiding this comment

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

Not a fan of this actually. Doing this makes the ARN a Fn::Join token and the loggroup name becomes a further token using more CF intrinsics.
While all of this is technically valid, it feels better, in this case, to tilt towards the reduction of token use.

I'd rather leave this as is.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@nija-at nija-at requested a review from eladb December 30, 2019 16:50
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

please update PR description per guidelines

@mergify
Copy link
Contributor

mergify bot commented Dec 31, 2019

Thank you for contributing! Your pull request is now being automatically merged.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • 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 Dec 31, 2019

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 9cbbaea into master Dec 31, 2019
@mergify mergify bot deleted the nija-at/logs-lgnameimport branch December 31, 2019 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants