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

Accept an optional fail-script parammeter #1374

Closed
danielo515 opened this issue Apr 13, 2018 · 9 comments
Closed

Accept an optional fail-script parammeter #1374

danielo515 opened this issue Apr 13, 2018 · 9 comments
Labels

Comments

@danielo515
Copy link
Contributor

Hello,

Lerna is a great tool, we do use it on our company on a daily basis. However, it seems to be focused on executing it from an interactive terminal by a human. The recent additions of auto-confirmation and that stuff have increased it's usability on automated scenarios, but there is still lacking some features for continuous integration environments.
For example, some errors are thrown, while some others are just displayed and the exit code is successful to the OS. This leads to pipelines that have some lerna/npm failure that are reported as successful, but this is just half of the story.
When something fails on an npm command you usually want to know why and maybe do something about it. Npm usually collects information about the error on log files, that are not showed by default. Of course it is impossible for lerna to provide an error handling behavior that satisfies all the users on all the possible scenarios.
That is why I'm proposing to accept an optional fail script that will be executed when some lerna command fails, no matter the reason (usually npm).
When lerna fails you just see some error trace and the usual node error that a rejected promise has not been caught. Since this is a rejected promise that will contain the error, I think it may be easy to attach a custom error handler for it, and allowing the end-user to provide this error handler will provide great flexibility.

I think the easiest approach would be to require this error-handling script to be a javascript file exposing a single function intended to receive and handle the error. This can allow the following scenarios:

  • Read the error message and show the error log file on console
  • Collect the logs and send them to a remote error processor
  • Analyze the error, try to correct it and re-launch the process
  • Send notifications about the error and terminate the process

Those are just some ideas I can come with, I'm pretty sure that allowing this kind of flexibility will make more people to come with smarter solutions.

Regard

@evocateur
Copy link
Member

We already have a --bail/--no-bail flag in a few commands, which defaults to true. I can see an argument for moving this to a global option, and modifying several loops and branches to consult the option to determine whether or not to re-throw caught errors.

Command Options:
  --bail  Stop when the command fails in a package.      [default: true]
          Pass --no-bail to continue despite failure.

The other bullet points seem way outside the scope of lerna itself, and would be more appropriate as a piped utility.

jenniesyip added a commit to jenniesyip/lerna that referenced this issue Apr 24, 2018
Keep seeing `--no-bail` in the issues, but it is not clearly documented. 

lerna#1374
lerna#1364
@oreporan
Copy link
Contributor

@evocateur
Question regarding the no-bail option
Does this cause the whole lerna execution to return exit code 0?
Or does it still return 1 if one of the execs returned 1?

@evocateur
Copy link
Member

Exit codes are not exactly correlated to --no-bail semantics. An unexpected error should always exit non-zero, though there are likely untested exceptions to this.

--no-bail was originally intended to allow lerna run and lerna exec to complete all of their chained executions regardless of individual script/shell exec failures. I want to say we're propagating any non-zero exit codes during a --no-bail command, but it is entirely likely they are being dropped unintentionally.

@oreporan
Copy link
Contributor

Yeah that's what Im seeing
I'd like to use no bail and let the packages exec, but still want to fail if any failed

I can PR it but it's obviously a change in behavior.
Any workaround you can recommend?

@evocateur
Copy link
Member

Dang, that's what I was afraid of. I can't think of a workaround off the top of my head, short of not passing --no-bail and letting lerna explode whenever a package exec fails...

I'd even argue that it's a proper bug fix to propagate the non-zero exit code. Despite technically being a breaking change, it's actually the expected result of the feature.

I guess we could add another flag, like --exit-code (naming is hard) to fix it without breaking. If --no-bail is passed without --exit-code, we should definitely warn extremely loudly about the lack of exit code preservation. Then in the next major version we make the correct behavior the default while removing the warning.

@spalladino
Copy link

Got here for the same exit code issue. I started using lerna run test --no-bail --concurrency=1 in my CI to run all tests, and I noticed that the build passed even if there were errors in some tests. I agree in that the expected behaviour should be to propagate the error as any non-zero exit code.

@oreporan
Copy link
Contributor

oreporan commented Sep 5, 2018

@spalladino @evocateur
I peeked around the code to try to make a quick PR, but to say the truth I couldn't find the place where to propgate the exit codes
I saw you're using the reject: true param in executing. But couldn't find where to resolve the exit codes
If someone wants to give it a go or point me to where this needs to be done

I'll create a separate issue for this

evocateur added a commit that referenced this issue Sep 6, 2018
This fixes `--no-bail` when used with `lerna exec` or `lerna run`,
ensuring that it still exits with a non-zero code in the event of a
command or script failure.

Refs #1374
Fixes #1653
@stale
Copy link

stale bot commented Dec 27, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 27, 2018
@stale stale bot closed this as completed Jan 3, 2019
@lock
Copy link

lock bot commented Apr 3, 2019

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants