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

Added local Mongo connection debugging tips #1184

Merged
merged 5 commits into from
Sep 4, 2019

Conversation

StephenWeatherford
Copy link
Contributor

Fixes #1137
Fixes #1000
Related to #706?

@StephenWeatherford StephenWeatherford requested a review from a team as a code owner September 3, 2019 21:47
Copy link
Contributor

@ejizba ejizba left a comment

Choose a reason for hiding this comment

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

Can we combine this logic into one function? You've got two catch blocks each with slightly different error handling, but I think they're meant to do the exact same thing


// Example error: "failed to connect to server [localhost:10255] on first connect [MongoError: connect ECONNREFUSED 127.0.0.1:10255]"
// Example error: "failed to connect to server [127.0.0.1:27017] on first connect [MongoError: connect ECONNREFUSED 127.0.0.1:27017]"
if (name === 'MongoError' && /ECONNREFUSED/.test(message) && /(localhost|127\.0\.0\.1)/.test(message)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer to use parseError

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 can't because that brings in vscode from the ui lib which doesn't work in the context of the language server, which uses this... :-) (Originally did it that way.)

@@ -75,7 +75,7 @@ export class MongoAccountTreeItem extends AzureParentTreeItem<IMongoTreeRoot> {
} catch (error) {
let message = parseError(error).message;
if (this._root.isEmulator && message.includes("ECONNREFUSED")) {
error.message = "Unable to reach emulator. Please ensure it is started and connected to the port specified by the 'cosmosDB.emulator.mongoPort' setting, then try again.\n" + message;
error.message = `Unable to reach emulator. See ${Links.LocalConnectionDebuggingTips} for debugging tips.\n${message}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use throw new Error( for this? I suppose this option is better because it maintains the original stack and error type, but it's definitely less robust

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is a hot bug fix, I only want to change what's absolutely necessary. (Actually this is a question I've waffled on, though - arguments on both sides.)

Copy link
Contributor Author

@StephenWeatherford StephenWeatherford left a comment

Choose a reason for hiding this comment

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

Can we combine this logic into one function? You've got two catch blocks each with slightly different error handling, but I think they're meant to do the exact same thing

I wanted to do that, that's why I was asking the question in e-mail. In the end, this caused too many changes (have to pass around isEmulator in lots of places, and it's got that weird code path where it's not set when I need it). I decided it was too risky to do right before releasing.

@StephenWeatherford StephenWeatherford merged commit 80e12b5 into master Sep 4, 2019
@StephenWeatherford StephenWeatherford deleted the saw/mongo-emulator branch September 4, 2019 20:20
@github-actions github-actions bot locked and limited conversation to collaborators Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants