Skip to content

Conversation

@ivikash
Copy link
Member

@ivikash ivikash commented Sep 25, 2024

Do not trigger uninstall when extension auto-updates

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ivikash ivikash requested a review from a team as a code owner September 25, 2024 21:58
@github-actions
Copy link

This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@ivikash ivikash force-pushed the fix/uninstall-handle-updates branch from 98f3749 to db04463 Compare September 25, 2024 21:59
@ivikash ivikash requested a review from a team as a code owner September 25, 2024 21:59
@ivikash ivikash force-pushed the fix/uninstall-handle-updates branch 3 times, most recently from 44a3594 to ef119b7 Compare September 25, 2024 22:07
async function handleUninstall(extensionFullName: string, callback: () => Promise<void>): Promise<void> {
await callback()
telemetry.aws_extensionUninstalled.run((span) => {
span.record({})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line necessary?

Copy link
Member Author

@ivikash ivikash Oct 3, 2024

Choose a reason for hiding this comment

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

Updated to telemetry.aws_extensionUninstalled.run(() => {})

* This makes sure the callback is triggered only when an explicit extension with specific version is uninstalled.
*/
const extension = `${extensionName}-${extensionVersion}`
const extensionFullName = `${extensionId}-${extensionVersion}`
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems inconsistent to pull extensionVersion from the environment, but you have to pass in the extension ID. The caller has the choice of what ID to pass but in reality since the version will only match to the running extension there is not actually a choice.
Why not pass in both or pull both from the env, e.g. vscode.ExtensionContext.extension.id.

@ivikash ivikash force-pushed the fix/uninstall-handle-updates branch from ef119b7 to 86956ff Compare October 1, 2024 23:10
@github-actions
Copy link

github-actions bot commented Oct 1, 2024

This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions.

@ivikash ivikash force-pushed the fix/uninstall-handle-updates branch from 86956ff to defd582 Compare October 3, 2024 03:48
@ivikash ivikash merged commit b6c40e1 into aws:master Oct 3, 2024
9 of 10 checks passed
@ivikash ivikash deleted the fix/uninstall-handle-updates branch October 3, 2024 17:38
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