-
Notifications
You must be signed in to change notification settings - Fork 128
feat: app list options and fixes to app and schema commands #101
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: app list options and fixes to app and schema commands #101
Conversation
packages/cli/src/lib/aws-utils.ts
Outdated
| if (!principal) { | ||
| principal = '148790070172' | ||
| } |
There was a problem hiding this comment.
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.
packages/cli/src/lib/aws-utils.ts
Outdated
| if (!principal) { | ||
| principal = '906037444270' | ||
| } | ||
|
|
||
| if (!statementId) { | ||
| statementId = 'smartthings' | ||
| } |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
| 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', | ||
| }), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| 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', | ||
| ] |
There was a problem hiding this comment.
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.
b42f9d3 to
1806acb
Compare
1806acb to
aa4a737
Compare
Several small fixes and enhancements:
appslist command to accept filter parameters.schema:updatewhere list selection wasn't working.