fix(connection-model): adjust connection for driver v4 COMPASS-4892

[mcasimir]

NOTE: please review also packages/connection-model/lib/connect.js which is collapsed.

• remove gssapiServiceName from the connection string, was already affected by https://jira.mongodb.org/browse/COMPASS-4529 that will have to be fixed in future.
• do not try to read SSL certs from files and just pass it to the driver instead
• don't set useNewUrlParser and useUnifiedTopology anymore as those options got removed.
• remove the adaptation of connection properties from 3.6 to 4.0 in compass shell
• refactor connection-model connect to use nowadays js instead of async and clean up dead code
• add a debug of the connection string (redacted) and options used by the driver

[gribnoysup]

LGTM, sweet connect method refactor!

Seems like windows is consistently fails to start mongodb server now in one of the tests, could you try starting a patch in Evergreen on Windows to check if that's a GitHub CI issue only? (mongodb-runner was flaky there before, maybe it's just shuffling the test order by removing some of them that increased the flakiness somehow)

[gribnoysup]

connect is an async function, wrapping done in nextTick feels a bit redundant here, right?

[mcasimir]

Yeah is probably unnecessary, that is to replicate the behaviour of util.callbackify and keep the interface outside 100% as it was. Basically if the handler fails the promise won't catch it and try to call another handler. At the end it doesn't really matter that much, but we would get an unhandled promise rejection instead of an uncaught exception.

[gribnoysup]

FWIW I was adding rebuild scripts only in post step for test suites that are running tests with electron runtime. Doesn't hurt to have it here, but might be a bit redundant

[mcasimir]

We really need to do the dynamic package that supports both electron and node when we have some time. The problem is that when you "develop" you'd end up mixing npm run start with CMD+R reloads (that does not rebuild) with npm run test, and you'd get basically skewed rebuilds that breaks the execution all the times. Since NAPI won't help till 2024 i guess the only option left is the double build trick that gets selected dinamically.

[gribnoysup]

I'd say it really depends on ones workflow then, I don't often jump between running tests and running an app so haven't noticed this an an issue. But to make it clear I am not dismissing this as an issue, let's open a ticket? We can also circumvent this in other ways, it's just if we will start adding rebuild before npm run test in more packages, it might slow down the tests (and CI flows) quite a bit

[gribnoysup]

Oh, also will add this to the docs

[mcasimir]

yeah i can create a ticket, the rebuild before tests should be a good idea at least for now, i bet there is a way to "detect" if a rebuild is required.

Another thing we could do is "flip" the logic. Right now we build for node and we keep it built for electron only while electron is running. We could do the other way around (build for electron by default) and only rebuild for node while running the tests.

After a while you get used to it but that issue is a bit of a deep thing to circumvent if you get it and don't have enough experience, i'm thinking about external or casual contributors / newbies too, and in general a random source of time wasted.

That's why I feel quite good about that "dynamic" require trick if we could timebox it to 1-2 days and would solve the issue forever. If it takes more or is problematic otherwise we could give up and fallback to rebuilds.

[gribnoysup]

Another thing we could do is "flip" the logic. Right now we build for node and we keep it built for electron only while electron is running. We could do the other way around (build for electron by default) and only rebuild for node while running the tests.

We can switch all tests to use electron-mocha as a test runner (a lot of packages are using it already) and then you don't need to rebuild anything back to node env at all I guess

[mcasimir]

We can switch all tests to use electron-mocha as a test runner (a lot of packages are using it already) and then you don't need to rebuild anything back to node env at all I guess

Ohh that i would like!! :)