Skip to content

Conversation

@bflorian
Copy link
Contributor

@bflorian bflorian commented Sep 3, 2020

Several small fixes and enhancements:

  • Added flags to apps list command to accept filter parameters.
  • Fixed bug in schema:update where list selection wasn't working.
  • Added flags to app and schema AWS lambda authorization to be able to override default principal and statement-id parameters.

@bflorian bflorian requested review from john-u and rossiam September 3, 2020 13:08
Comment on lines 42 to 44
if (!principal) {
principal = '148790070172'
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here too, principal can't be undefined because of the defaulted parameter.

Comment on lines 11 to 17
if (!principal) {
principal = '906037444270'
}

if (!statementId) {
statementId = 'smartthings'
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You actually don't need this because you have the default values. (Even passing a literal undefined as one of the values will use its default.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha! I was thinking it would behave like a null, but not setting a flag does return undefined, so I removed these for clarity

Comment on lines 19 to 24
principal: flags.string({
description: 'use this principal instead of the default when authorizing lambda functions',
}),
'statement-id': flags.string({
description: 'use this statement id instead of the default when authorizing lambda functions',
}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these flags are used so often, It might be worth putting these in some common place and ...ing them in so if we need to update the text, we can just do it in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I moved them

Comment on lines +26 to +32
static examples = [
'$ smartthings apps:authorize arn:aws:lambda:us-east-1:1234567890:function:your-test-app',
'',
'Note that this command is the same as running the following with the AWS CLI:',
'',
'$ aws lambda add-permission --region us-east-1 \\',
' --function-name arn:aws:lambda:us-east-1:1234567890:function:your-test-app \\',
' --statement-id smartthings --principal 148790070172 --action lambda:InvokeFunction',
'',
'It requires your machine to be configured to run the AWS CLI',
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love that you're including these examples. I need to do that more often.

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.

2 participants