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

feat(logs): import a LogGroup from its name #5580

Merged
merged 4 commits into from
Dec 31, 2019
+39 −2
Merged

feat(logs): import a LogGroup from its name #5580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8146573
feat(logs): import a LogGroup from its name
Dec 30, 2019
4916c66
reuse fromLogGroupArn
Dec 30, 2019
6964353
Revert "reuse fromLogGroupArn"
Dec 30, 2019
e7eb374
Merge branch 'master' into nija-at/logs-lgnameimport
mergify[bot] Dec 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion packages/@aws-cdk/aws-logs/lib/log-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ export interface LogGroupProps {
*/
export class LogGroup extends LogGroupBase {
/**
* Import an existing LogGroup
* Import an existing LogGroup given its ARN
*/
public static fromLogGroupArn(scope: Construct, id: string, logGroupArn: string): ILogGroup {
class Import extends LogGroupBase {
Expand All @@ -316,6 +316,23 @@ export class LogGroup extends LogGroupBase {
return new Import(scope, id);
}

/**
* 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 •
edited
Loading

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.

public readonly logGroupName = logGroupName;
public readonly logGroupArn = Stack.of(scope).formatArn({
service: 'logs',
resource: 'log-group',
sep: ':',
resourceName: logGroupName,
});
}

return new Import(scope, id);
}

/**
* The ARN of this log group
*/
Expand Down
22 changes: 21 additions & 1 deletion packages/@aws-cdk/aws-logs/test/test.loggroup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export = {
test.done();
},

'export/import'(test: Test) {
'import from arn'(test: Test) {
// GIVEN
const stack2 = new Stack();

Expand All @@ -119,12 +119,32 @@ export = {
imported.addStream('MakeMeAStream');

// THEN
test.deepEqual(imported.logGroupName, 'my-log-group');
test.deepEqual(imported.logGroupArn, 'arn:aws:logs:us-east-1:123456789012:log-group:my-log-group');
expect(stack2).to(haveResource('AWS::Logs::LogStream', {
LogGroupName: "my-log-group"
}));
test.done();
},

'import from name'(test: Test) {
// GIVEN
const stack = new Stack();

// WHEN
const imported = LogGroup.fromLogGroupName(stack, 'lg', 'my-log-group');
imported.addStream('MakeMeAStream');

// THEN
test.deepEqual(imported.logGroupName, 'my-log-group');
test.ok(/^arn:.+:logs:.+:.+:log-group:my-log-group$/.test(imported.logGroupArn),
`LogGroup ARN ${imported.logGroupArn} does not match the expected pattern`);
expect(stack).to(haveResource('AWS::Logs::LogStream', {
LogGroupName: 'my-log-group'
}));
test.done();
},

'extractMetric'(test: Test) {
// GIVEN
const stack = new Stack();
Expand Down