Crash when MinIO bucket does not have versioning enabled #545

[fflorent]

Fixes #545

Notes for the reviewers:

• I assumed the ICreate#configure() method is to proceed to some checks before installing the document-related endpoint, based on this comment, is it OK for you?
• As an exception in the configure method would not stop the server, I rather called process.exit(). However I feel like just throwing an exception (and thus disallowing using the /docs endpoint) can be also reasonable.
• It may also need some tests. I will write them when the main idea of the implementation is stable (especially when my questions above and this comment are answered :) ).

Thanks in advance for your help 🙏

[paulfitz]

* I assumed the `ICreate#configure()` method is to proceed to some checks before installing the document-related endpoint, [based on this comment](https://github.com/incubateur-territoires/grist-core/blob/a5998bbcfdc56b1203c576da7a287e24b0223733/app/server/lib/ICreate.ts#L33), is it OK for you?

That's a fine interpretation.

* As an exception in the configure method would not stop the server, I rather called `process.exit()`. However I feel like just throwing an exception (and thus disallowing using the `/docs` endpoint) can be also reasonable.

I'm surprised that throwing an exception in configure wouldn't propagate to mergedServerMain.main and so prevent any server that includes docs endpoints from starting. I would prefer an exception if possible.

* It may also need some tests. I will write them when the main idea of the implementation is stable (especially when my questions above and [this comment](https://github.com/gristlabs/grist-core/pull/546/files#r1242264301) are answered :) ).

Main idea seems grand :-) - thanks for working on this!

[fflorent]

I'm surprised that throwing an exception in configure wouldn't propagate to mergedServerMain.main and so prevent any server that includes docs endpoints from starting. I would prefer an exception if possible.

I made a change here where the main server is called:
76df23c

Whether I start using yarn start or yarn start:prod, it seems that the stubs/app/server/server.ts file is executed

[paulfitz]

Ah, sounds like node is waiting for something to finish up. Maybe mergedServerMain.main needs a catch clause that calls server.close() to shut down anything that is already started, if there is an exception.

[fflorent]

Ah, sounds like node is waiting for something to finish up. Maybe mergedServerMain.main needs a catch clause that calls server.close() to shut down anything that is already started, if there is an exception.

I attempted the below patch, with no luck, the process does not exit despite the server being closed:
incubateur-territoires@f46bbe4

[paulfitz]

Ah, sounds like node is waiting for something to finish up. Maybe mergedServerMain.main needs a catch clause that calls server.close() to shut down anything that is already started, if there is an exception.

I attempted the below patch, with no luck, the process does not exit despite the server being closed: incubateur-territoires@f46bbe4

Hmm, if I throw an exception just before the call to configure(), when using your branch, behavior with yarn run start:prod looks as expected (exception thrown, process stops). Haven't tried with MinIO actually set up though...

[fflorent]

You're right, that works in production mode and that seems reasonable like this. I cherry-picked the commit and removed the await server.stopListening('crash'); instruction as close() makes the app crash.

[paulfitz]

Looks good, thank you! A minio server is set up in .github/workflows/main.yml and some tests are run against it, so a test could be added for this fail-fast feature. But it feels like it could be a bit cumbersome, and the consequences of failure seem very minor, I'm ok accepting this without automated tests.

[paulfitz]

@fflorent let me know if you'd like this landed as is, or if you have ambitions to write a test for it (your call, not essential in this case in my opinion).

[fflorent]

OK then, I am OK merging without tests.