Fail if any checks were failures #46
Conversation
|
Thanks for this! I just had a really quick look at this PR. This behavior HAS to be controlled via a command-line flag. Based on my experience, on CI you never want the whole CLI to fail. Often CC can be run as a single step in a bigger job. I get that sometimes it might be useful (especially for running checks locally) to return non-zero exit code but I don't believe that it should be the default behavior. Also then it won't be a breaking change ;) |
|
@krzkaczor I thought about it and I'm not so sure it should be behind a feature flag. When I run my tests and a test fails I want CI to fail (and maybe not deploy if that's part of the pipeline). Why would I like it to work differently in case of codechecks? I think this would be the users' expectation and it's better to be safe than sorry (and fail the CI).
Yes, and then you can manage dependencies on different steps in your CI of choice. I'm happy to make a new flag but I would personally prefer that using it returns 0 when there're failures than return non-0 code on failure. In other words, I believe it should be the default behavior, not an opt-in. Also, you have a complex CI? You can figure out the flag. |
packages/client/src/runner.ts
Outdated
| logger.log(`${bold("Checks:")} ${result}`); | ||
| logger.log(`${bold("Time:")} ${ms(deltaTime)}`); | ||
|
|
||
| if (sharedExecutionCtx.isWithExitStatus && failureCodechecks > 0) { |
There was a problem hiding this comment.
I believe here we could access isWithExitStatus directly from args.
There was a problem hiding this comment.
I'm hesitant to use this directly from args. I don't want to use it directly to avoid refactorings when the same information can, not only be set via process args, but also configured in codechecks.yml or using an environment variable. These are fairly plausible ways to influence the way CC can be run (in the future).
I can come up with the abstraction layer for config instead.
There was a problem hiding this comment.
I can come up with the abstraction layer for config instead.
yes this seems like a way to go 👍
|
@MichalZalecki I hear you. As I said, this was how I always imagined it to work, but it's not necessarily the right view 😆 I would put it as another input for 0.2.0 release and have hopefully more people wage into this. I feel like it's a much larger discussion - should CC be just a suggestion for a reviewers or should it really block CI pipeline. |
|
@krzkaczor It's only a thin abstraction but allows runner-only options can be kept as part of |
|
Thanks for this! Merging! |
This one is intended to finally fix #22. I consider this a breaking change (duh!).