-
Notifications
You must be signed in to change notification settings - Fork 744
feat(amazonq): display transformation history and add ability to resume interrupted jobs #7781
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(amazonq): display transformation history and add ability to resume interrupted jobs #7781
Conversation
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
| // Convert milliseconds to days (1000ms * 60s * 60min * 24hr) | ||
| const daysDifference = timeDifference / (1000 * 60 * 60 * 24) | ||
|
|
||
| return daysDifference <= 30 |
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.
Do we need to explain why 30?
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'll just provide some context in this reply: we decided to only show users jobs from up to 30 days ago because on our backend we only store jobs and artifacts for up to 30 days. This way we align with that storage policy.
| void setContext('gumby.wasQCodeTransformationUsed', false) | ||
|
|
||
| const transformationHubViewProvider = new TransformationHubViewProvider() | ||
| const transformationHubViewProvider = TransformationHubViewProvider.instance |
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.
Can we just explain this for context?
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.
Using TransformationHubViewProvider.instance here to construct the instance implements the singleton pattern and ensures that only one instance exists throughout. That way, when TransformationHubViewProvider.instance is referenced in different parts of the code, we get the same instance every time. I was having issues with accessing the instance with the way it was constructed previously since there seemed to be two instances that existed at the same time.
| transformByQState.setPolledJobStatus('FAILED') | ||
| // jobFailureErrorNotification should always be defined here | ||
| const displayedErrorMessage = | ||
| transformByQState.getJobFailureErrorNotification() ?? CodeWhispererConstants.failedToCompleteJobNotification |
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.
Can we just add some context to the PR why we are getting rid of these so reviewers are aware?
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.
We decided to remove the stopJob() call because with the introduction of the refresh functionality, users can actually resume their jobs within 12 hours. This call was initially added so that jobs that were interrupted on client side would not just run till they time out on the backend. However, with this feature, since users can now continue their jobs, stopping the job on the backend would prevent users from taking this action.
| : this.getTableMarkup(sessionJobHistory[transformByQState.getJobId()]) | ||
| : this.getTableMarkup(jobsToDisplay) | ||
| } | ||
| <script> |
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 curious, are there other examples of us returning script tags within HTML?
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.
There are a few that I can see: showPlanProgress() method in TransformationHubViewProvider.ts, getWebviewContent() method in webViewPanel.ts -- this one uses it similarly to how I have done so here --, and getHtml() method in referenceLogViewProvider.ts
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Show resolved
Hide resolved
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
| }) | ||
|
|
||
| // create local history folder(s) and store metadata | ||
| const jobHistoryPath = path.join(os.homedir(), '.aws', 'transform', transformByQState.getProjectName(), jobId) |
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.
Is this the same folder as where the QCT CLI stores artifacts?
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.
No, the CLI stores artifacts in .aws/qcodetransform/ (specifically within the transformation_projects subdirectory)
| if ( | ||
| transformByQState.isSucceeded() || | ||
| transformByQState.isPartiallySucceeded() || | ||
| transformByQState.isCancelled() |
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.
Instead of checking the job status to delete these files, just check if fs.existsSync() to determine whether or not to delete the files
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.
The reason I was thinking of using status as the check here is because if the job fails or is incomplete on client side, we do not want to delete the metadata file and zip copy since those are needed to resume the job. Is there a better way to do this check?
Sidenote: I will be moving the original zip deletion (lines 757-760) outside of this if clause since that can be deleted regardless
| ? 'disabled title="A job is ongoing"' | ||
| : job.status === 'CANCELLED' || | ||
| job.status === 'STOPPED' || | ||
| job.status === 'FAILED_BE' |
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.
How is FAILED_BE managed? We already have FAILED so wondering if we need this
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 added FAILED_BE for the case where a job actually fails on the backend, though users will still only see it show up as FAILED in the Transformation Hub. In such a case we do not want users to be able to refresh that job because there are no updates for it on the backend. I had to add this local status because when checking for any jobs that may need to be resumed, I search for a status of FAILED in the local transformation history file (since this is the status that shows up on the frontend in the case of a client-side failure, like a network disruption). We don't want to mislead users into thinking an actual failed job is still refreshable or "in progress," so this FAILED_BE status helps differentiate that. But it's only used in the context of the history file/table.
Perhaps there is a better way I could've done this?
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
| // artifacts should be available to download | ||
| jobHistoryPath = await this.retrieveArtifacts(jobId, projectName) | ||
|
|
||
| // delete metadata and zipped code files, if they exist |
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.
what happens if they don't exist?
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.
Nothing happens; since I'm using { force: true } here, no error is thrown even if the files don't exist
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
8e20bee to
1609879
Compare
d9ccc41 to
ad8ef31
Compare
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
ad8ef31 to
a50281b
Compare
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Outdated
Show resolved
Hide resolved
packages/core/src/test/amazonqGumby/transformationJobHistory.test.ts
Outdated
Show resolved
Hide resolved
6689e8f to
0b29f26
Compare
…tches locally; write history details to local log file
…diff patch if available), also added isWithin30Days util func for future use
…build log; open diff patch and summary directly in VSCode
…less often, limit table to 10 rows and validate dates
… transform initiated; create unit tests for job history feature
0b29f26 to
31bd370
Compare
|
/retryBuilds |
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
|
/retryBuilds |
Problem
Users cannot see previous job details (status, project name, job id, etc) and cannot access the final diff patch. Network issues can cause jobs to fail on client side while they continue on the backend, and users have no way to access those artifacts.
Solution
Repurpose the job status table to show most recent 10 jobs run in the last 30 days, including links to final diff patch and summary files. Allow users to retrieve missing artifacts for jobs and to resume incomplete jobs via refresh button.
feature/xbranches will not be squash-merged at release time.