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

chore: generate test connections COMPASS-7546 #25

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

mabaasit
Copy link
Contributor

As we removed import-test-connections script from Compass (in mongodb-js/compass#5247), we added this code here.

Copy link
Contributor

@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.

Do you think it's at all possible that this script autoimports connections using cli commands and generated file like the old script did?

@mabaasit
Copy link
Contributor Author

Do you think it's at all possible that this script autoimports connections using cli commands and generated file like the old script did?

No it does not do that (import of the connections). I did not find any usage of our script within Compass codebase. Are we using it somewhere else? Or we just keep the script so that we can quickly import test connections in Compass?

@gribnoysup
Copy link
Contributor

I did not find any usage of our script within Compass codebase

Super sorry, I think I'm not explaining clearly enough here what I'm asking about. What I mean is that this old script that we're removing was made for convenience of the engineers, not for some CI task or something. If you want to prepopulate your local Compass installation with connections instead of doing this manually, the script will do this for you with just one command, it will put all the files in correct place and you can start Compass and see your connections, all done from just compass monorepo, one step to do this, pretty cool and convenient. Now that we moved it to this repo, there are multiple steps spread across multiple repos and manual import in the application.

I'm not saying this was exactly the old behavior, I'm just asking whether or not you think it's feasible to implement something that is as convenient as the old script was? All good if not easy to do, we can tackle it in the future

@mabaasit
Copy link
Contributor Author

Super sorry.

I actually ended on your comment on another PR (related to this one) and that made things clear. It makes sense to add this. I'll be making this change.

@mabaasit mabaasit force-pushed the generate-test-connections branch from 6300ec7 to 4a1f42e Compare December 19, 2023 19:28
src/index.ts Outdated
connections.push({
id: v4(),
favorite: {
name: variant,
Copy link
Member

Choose a reason for hiding this comment

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

We're using some internal Compass format here, would it be better if we instead have this on the Compass side? I did approve the removing of the test connections in mongodb-js/compass#5247 Looking at it more I'm thinking it could make more sense to have that script still in Compass and do this connection loading into Compass' format there instead of here.
The functionality Sergey mentioned of loading the test connections is a nice one for setting up on new environments.

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, agreed. However this came up in a slack conversation and we agreed to remove the script from Compass completely and have a set connections (json) in this repo, but I ended up creating a script.

I added another comment on the related PR in Compass (sorry the discussion is split across).

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have an public import connection file format, this is what is used by import-export-connections, so I'd just update the script to generate the file in this format so that you can then import it in compass (I kinda assumed that this is what this script is already generating, didn't notice that it's missing a few fields from the exported connections file)

@mabaasit mabaasit force-pushed the generate-test-connections branch from 4a1f42e to b38b1d6 Compare December 20, 2023 13:30
const path = require('path');
const fs = require('fs/promises');

const FILE_PATH = path.resolve(__dirname, '..', 'compass-connections.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit: we probably want to write this in the same folder where you ar running this from (or from an argument with this being a default, but this complicates things too much)

Suggested change
const FILE_PATH = path.resolve(__dirname, '..', 'compass-connections.json');
const FILE_PATH = path.resolve(process.cwd(), 'compass-connections.json');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed this one. will add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in #27

@mabaasit mabaasit merged commit 63f487a into main Dec 21, 2023
1 check passed
@mabaasit mabaasit deleted the generate-test-connections branch December 21, 2023 13:00
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.

3 participants