feat: add console command to storage category #10981
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #10981 +/- ##
==========================================
+ Coverage 47.82% 47.85% +0.03%
==========================================
Files 669 670 +1
Lines 32678 32719 +41
Branches 6608 6614 +6
==========================================
+ Hits 15627 15659 +32
- Misses 15419 15428 +9
Partials 1632 1632
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Hey @fossamagna thanks for the contribution! Functionally this change looks good to me, just a couple small nits to address.
| @@ -41,8 +44,11 @@ export { | |||
| s3RemoveStorageLambdaTrigger, | |||
| } from './provider-utils/awscloudformation/service-walkthroughs/s3-resource-api'; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
nit: prefer to disable this lint rule for the file rather than have empty comment blocks
| return; | ||
| } | ||
| const { tableName, region } = await prompter.pick<'one', Pickchoice['value']>("Select DynamoDB table to open on your browser", tables); | ||
| const url = `https://${region}.console.aws.amazon.com/dynamodbv2/home?region=${region}#table?initialTagKey=&name=${tableName}&tab=overview`; |
There was a problem hiding this comment.
is the initialTagKey= query parameter needed?
There was a problem hiding this comment.
Its query parameter is not required, I will remove it.
|
@edwardfoyle Thank you for your review. |
it is not required.
fossamagna
force-pushed
the
feat/storage-console-command
branch
from
46e7784
to
2fefad9
Compare
| printer.info(err.stack); | ||
| printer.error('There was an error trying to open the storage web console.'); | ||
| throw err; |
There was a problem hiding this comment.
Apologies for not catching this in the previous review. We have a new top-level error handler that will take care of printing stack traces and printing messages. This try/catch can be removed and the error handler will handle printing the message and the stacktrace
There was a problem hiding this comment.
@edwardfoyle Thank you for your review.
I could not find where top-level error handling is. Am I missing somewhere?
Would you tell me where top-level error handling takes place?
There was a problem hiding this comment.
Would you tell me where top-level error handling takes place?
I found the top-level error handling.
There was a problem hiding this comment.
@edwardfoyle I removed try-catch to use top-level error handling.
|
@edwardfoyle I fixed according to your addition suggestions. Could you review this RP again ? |
|
@edwardfoyle Is this RP has blockers? Is there anything I can do to get my PR reviewed? |
|
@edwardfoyle I fixed according to your addition suggestions. Could you review this RP again ? |
There was a problem hiding this comment.
Apologies for the slow turnaround @fossamagna! I found a couple things I think need updating, but otherwise it LGTM. Thank you again for your contribution!
|
@jhockett Thank you for your review. I corrected it in accordance with your suggestions. |
fossamagna
requested review from
jhockett
and removed request for
edwardfoyle
|
This looks good to me. I do see that the |
|
@goldbez Thanks. I run |
Thank you |
|
Thank you for your contribution, @fossamagna |
Implements
amplify storage consolecommand.Description of changes
Issue #, if available
fix #9900
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.