-
Notifications
You must be signed in to change notification settings - Fork 595
fix(amazonq): add a more descriptive issue for firewall problems #7251
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
|
@@ -201,12 +202,6 @@ export function createLspInstallerTests({ | |||
id: lspConfig.id, | |||
manifestLocation: 'remote', | |||
languageServerSetupStage: 'getManifest', | |||
result: 'Failed', |
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.
this was broken in a different commit. Now the manifest download falls back to the local cache if an etag is found but it still reports the telemetry metric as "remote"
Technically this is a minor bug with reporting telemetry, since it should have been reported as a cache
@@ -69,7 +69,6 @@ export class ManifestResolver { | |||
|
|||
const localManifest = await this.getLocalManifest(true).catch(() => undefined) | |||
if (localManifest) { | |||
localManifest.location = 'remote' | |||
return localManifest |
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 set location=remote
, is because this logic is in the fetchRemoteManifest
function, and semantically, this is equivalent to when a "fetch" happens. It's just that fetch was skipped in this case, because the server version is the same.
My intuition was that location=local
means a local fallback was used instead of the current remote version. I guess it could be interpreted both ways though.
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.
nice!
throw new ToolkitError( | ||
`Unable to download dependencies from ${this.manifestUrl}. Check your network connectivity or firewall configuration and then try again.`, | ||
{ | ||
code: 'NetworkConnectivityError', |
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.
these error codes auto-populate the reason
field in telemetry right? If so, this will be important to track going forward to gauge impact. My suspicion is this only affects a very low % of users.
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.
Yeah exactly, that's part of why I wanted to add the code here. It should give us a rough idea of how many people are effected
Problem
If a users firewall is blocking downloading the zips or reaching the manifest its not clear to the user what to do
Solution
If the user:
it might be a firewall issue. Rather than just
Unable to find a compatible version of the Language Server
we show a more descriptive message to the userfeature/x
branches will not be squash-merged at release time.