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

Why do postinstall scripts exit with code 0 on failure? #442

Open
mhassan1 opened this issue Feb 22, 2021 · 6 comments
Open

Why do postinstall scripts exit with code 0 on failure? #442

mhassan1 opened this issue Feb 22, 2021 · 6 comments

Comments

@mhassan1
Copy link
Contributor

Versions

  • NodeJS: 14.15.1
  • mongodb-memory-server-*: 6.9.3
  • mongodb: 4.4.1
  • mongoose: 5.11.15
  • system: MacOS

package: mongodb-memory-server

What is your question?

In the postinstall script in mongodb-memory-server, there are a few failure conditions that result in an exit code of 0, for example:

I see related discussion here: #131

IMO, the postinstall should not exit with code 0 when the script fails to do what the consumer expects. Presumably, consumers are using the mongodb-memory-server package because they want the postinstall step to happen; if they didn't care about that, then they would use mongodb-memory-server-core, instead. If the postinstall fails, they should be notified with a non-zero exit code; if they don't like that behavior, they can opt for the core package, or we can offer them another way to opt out of that behavior.

What was the reasoning behind exiting with code 0 for failure cases?

@hasezoey
Copy link
Member

as an note, you were looking at outdated files (but the point still stands):

this is currently to not block the install of the packages (might be counter intuitive)

@nodkz do you think we should actually fail in the postinstall? (aside from "skip" & "systembinary")

@nodkz
Copy link
Collaborator

nodkz commented Feb 22, 2021

Motivation:

  • supposemongodb-memory-server block packages installation by default – if some package blocks installation I remove it from my dependencies. And most developers will do the same. Also, they do not run tests often, why they should be injured and their work halted by some package? And mongodb binary upstream sometimes has problems with DNS.
  • if postinstall fails and does not download binaries – then binaries will be downloaded on test start. They are checked every time before the test starts and download binaries if they are missing.

We can add a new option for package.json or env variable e.g MONGOMS_FAIL_ON_POSTINSTALL or something similar. And in this case on postinstall exit with non-zero code. So you will be able to explicitly enable this option with tough behavior 😉

@mhassan1 @hasezoey how do you think this option/env can be named? Is MONGOMS_FAIL_ON_POSTINSTALL good enough?

@mhassan1
Copy link
Contributor Author

mhassan1 commented Feb 22, 2021

Personally, I think the default behavior should be to fail, with an option like MONGOMS_IGNORE_POSTINSTALL_ERRORS if you want to ignore it temporarily, but I won't argue 😉.

I think something like MONGOMS_FAIL_ON_POSTINSTALL_ERRORS is more explicit.

@hasezoey
Copy link
Member

isnt MONGOMS_IGNORE_POSTINSTALL_ERRORS basically MONGOMS_DISABLE_POSTINSTALL?

and when implementing MONGOMS_IGNORE_POSTINSTALL_ERRORS or MONGOMS_FAIL_ON_POSTINSTALL_ERRORS, what should be do about MONGOMS_DISABLE_POSTINSTALL, keep it or remove it?

@mhassan1
Copy link
Contributor Author

Good point. There's not much of a difference between MONGOMS_IGNORE_POSTINSTALL_ERRORS and MONGOMS_DISABLE_POSTINSTALL.

@hasezoey
Copy link
Member

actually, when implementing a way to not fail with code 0, we could maybe add an test that creates an temporary directory and an basic package.json and try to install everything (with local linking install)

@github-actions github-actions bot added the stale This Issue is outdated and will likely be closed if no further comments are given label Apr 26, 2021
@hasezoey hasezoey added enhancement and removed question stale This Issue is outdated and will likely be closed if no further comments are given labels Apr 26, 2021
@typegoose typegoose deleted a comment from github-actions bot Apr 26, 2021
@hasezoey hasezoey mentioned this issue May 10, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants