Skip to content
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): Auto update language servers when new versions are available #6310

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

jpinkney-aws
Copy link
Contributor

@jpinkney-aws jpinkney-aws commented Jan 6, 2025

Problem:

  • Only one version of a language server is installed right now

Solution:

  • Automatically update when a new version is available in the manifest

TODO

  • install(manifest) should be install(version) which would clean some things up
  • aws.toolkit.lsp.manifest should have named manifests, since every language server could have a different manifest
  • e2e tests for actually downloading manifests and checking that they're ready and available
  • figure out if we only update when vscode restarts or how we want to handle that
  • re-add cleanup
  • telemetry for downloading/updating the language server

  • 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.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ilable

Problem:
- Only one version is installed right now

Solution:
- Automatically update when a new version is available in the manifest
@jpinkney-aws jpinkney-aws requested review from a team as code owners January 6, 2025 17:16
Copy link

github-actions bot commented Jan 6, 2025

  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@jpinkney-aws jpinkney-aws marked this pull request as draft January 7, 2025 18:36
packages/core/src/shared/languageServer/lspManager.ts Outdated Show resolved Hide resolved
packages/core/src/shared/languageServer/lspManager.ts Outdated Show resolved Hide resolved
packages/core/src/shared/languageServer/lspManager.ts Outdated Show resolved Hide resolved
packages/core/src/shared/languageServer/lspManager.ts Outdated Show resolved Hide resolved
packages/core/src/shared/utilities/download.ts Outdated Show resolved Hide resolved
packages/amazonq/src/lsp/lspInstaller.ts Outdated Show resolved Hide resolved
packages/core/src/shared/languageServer/lspManager.ts Outdated Show resolved Hide resolved
packages/core/src/shared/languageServer/manifestManager.ts Outdated Show resolved Hide resolved
await fs.delete(tempFolder)
})

describe('download', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests can likely be made obsolete by testing the same scenarios in tests for AmazonQLSPInstaller.
Stubbing the http fetcher should be enough to test the entire flow, and then you could have complex tests where it first downloads from the remote, then a subsequent call should instead use the cache

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah now that the installer exists I can bring them up a level. I'll add that as a followup item

@@ -201,6 +202,50 @@ export class HttpResourceFetcher implements ResourceFetcher {
}
}

export class RetryableResourceFetcher extends HttpResourceFetcher {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should probably be done in a different way. Maybe the httpResourceFetcher can have retries in it? I think we can re-visit this after this PR

@@ -270,3 +271,14 @@ export async function getMachineId(): Promise<string> {
// TODO: check exit code.
return (await proc.run()).stdout.trim() ?? 'unknown-host'
}

export function getApplicationSupportFolder() {
switch (process.platform) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO support more platforms and their fallback locations

new Range(supportedLspServerVersions)
).resolve()

// TODO Cleanup old versions of language servers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling this out explicitly so we don't forget

}

const downloadTasks = contents.map(async (content) => {
// TODO This should be using the retryable http library but it doesn't seem to support zips right now
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling this out as well, we should switch to something that has retries with backoffs

@nkomonen-amazon nkomonen-amazon marked this pull request as ready for review January 8, 2025 21:31
Copy link
Contributor

@nkomonen-amazon nkomonen-amazon left a comment

Choose a reason for hiding this comment

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

Approved as this code does not impact prod and we can do fast follow ups

@jpinkney-aws jpinkney-aws merged commit 20efa94 into aws:feature/amazonqLSP Jan 8, 2025
5 of 17 checks passed
@jpinkney-aws jpinkney-aws deleted the updating-lsp branch January 8, 2025 22:07
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