Skip to content

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

Merged
merged 2 commits into from
Apr 15, 2025
Merged

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Apr 15, 2025

Atlas Metadata is not set on Compass Desktop environments, so instead this determines it through the connection string.

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.
@github-actions github-actions bot added the fix label Apr 15, 2025
@gagik gagik added no release notes Fix or feature not for release notes no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) labels Apr 15, 2025
Comment on lines 1562 to 1564
isAtlas: isAtlas(
connectionInfo.connectionOptions.connectionString
),
Copy link
Collaborator

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?

@gribnoysup
Copy link
Collaborator

Atlas Metadata is never set before setting a connection

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

Copy link
Collaborator

@gribnoysup gribnoysup left a 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

@gagik
Copy link
Contributor Author

gagik commented Apr 15, 2025

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

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.
Copy link
Contributor Author

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

@gagik gagik merged commit 870bce1 into main Apr 15, 2025
57 checks passed
@gagik gagik deleted the gagik/fiix-appname branch April 15, 2025 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix no release notes Fix or feature not for release notes no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants