-
Notifications
You must be signed in to change notification settings - Fork 214
fix: determine Atlas connection from connection String #6867
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
Atlas Metadata is never set before setting a connection so it is erroneous to use it when connecting. This uses a utility method instead that checks the connection string.
isAtlas: isAtlas( | ||
connectionInfo.connectionOptions.connectionString | ||
), |
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 this be completely encapsulated inside the adjustConnectionOptionsBeforeConnect
method? Is there any reason to keep this logic configurable from outside if we always want this to be applied based on the connection string?
Just so it doesn't cause you confusion in the future, the metadata is always set on the connectionInfo object if this is a connection info returned by Atlas control plane, this is only applicable to compass-web that is integrated in the mms at the moment though, never compass desktop |
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.
Looks good, but I do have a question if we need to keep this configurable from outside if this logic should always activate if applicable and the detection method depends only on the values that we already pass to the method
Ah that makes sense, thank you, I had trouble following how it ends up set. Maybe I could add that as comment on the field to make that clearer |
@@ -136,7 +136,7 @@ export interface ConnectionInfo { | |||
connectionOptions: ConnectionOptions; | |||
|
|||
/** | |||
* The metdata for the Atlas cluster | |||
* The metadata for the Atlas cluster. Set from Atlas control plane when using compass-web. |
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.
@gribnoysup just added this comment from better awareness
Atlas Metadata is not set on Compass Desktop environments, so instead this determines it through the connection string.