-
Notifications
You must be signed in to change notification settings - Fork 750
feat(amazonq): auto-download results #6519
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
Conversation
|
|
|
||
| // attempt download for user | ||
| if (transformByQState.isSucceeded() || transformByQState.isPartiallySucceeded()) { | ||
| await vscode.commands.executeCommand('aws.amazonq.transformationHub.reviewChanges.startReview') |
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.
Just as a random aside, ideally this would have used our internal Commands wrapper core/src/shared/vscode/commands2.ts. This wrapper basically allows you to define the command as a const that you can execute later which is much more robust then having to pass around command ids!
In order to do that you would need to register aws.amazonq.transformationHub.reviewChanges.startReview like:
export const startReview = Commands.declare('aws.amazonq.transformationHub.reviewChanges.startReview',
() => async () => {
// implement me here
}
)
and then here you could just do startReview.tryExecute() that way you don't have to care about underlying id's!
See: https://github.com/aws/aws-toolkit-vscode/blob/master/docs/arch_develop.md#commands for more info
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.
Noted, https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/codewhisperer/service/transformByQ/transformationResultsViewProvider.ts is full of registered commands that could be refactored this way, so I think it's better if I do all of that in a separate PR.
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'm okay with merging this as is as long as you commit/agree to making that change 😄. It should be like 15 minutes worth of work tops
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.
Added a TODO comment and made a note of it so I don't forget
## Problem Transformation results/artifacts expire after 24h, and sometimes users forget to manually click the "Download Proposed Changes" button within that time period. ## Solution Auto-download the results from S3 when ready. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: David Hasani <[email protected]>
Problem
Transformation results/artifacts expire after 24h, and sometimes users forget to manually click the "Download Proposed Changes" button within that time period.
Solution
Auto-download the results from S3 when ready.
feature/xbranches will not be squash-merged at release time.