-
Notifications
You must be signed in to change notification settings - Fork 830
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
Allow thresholds to be empty #725
Conversation
Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests. |
Oh, sorry that we got that wrong. I'm fine with changing the signatures to |
Yes it will break the backward compat, but in this case this is a good thing. Let me try to convince you :) The contribution guide states that "Every option should have the same defaults as the UI", which is not the case here. I personally spent quite some time trying to figure out why the plugin was behaving so strangely in a generated job. I think it might be better to endure the pain now and make people change their code than to continue to confuse the newcomers, of which there will be much more :) Let me also say that I understand the importance of not breaking the existing code, but I see that the quality of the product and the codebase still takes first priority here (judging by the deprecation policy for example), which is why I ask you to consider breaking the compatibility in this instance. Still, I am not opposed to leaving the defaults in place (we're building the Job DSL from our own fork anyway), but if you think that we absolutely should not change them, maybe we could do something clever and think of a way to deprecate the current defaults and introduce a way to switch to the new ones, allowing people to use them? |
I will not merge an incompatible change. The docs clearly state that the default value is zero. It's a bit unfortunate in this case that this does not match the plugin's default, but potentially breaking user code is even worse. Restore the default values or I will close the PR. |
3ec14c6
to
01e7704
Compare
Change-Id: I203a6f3e7b22d2238f71b9efe19c6da2778cb104
01e7704
to
fac70e0
Compare
I still believe that changing the defaults would be better for the product, but it is your call :) I've changed them back. Thanks for reviewing the pull request! |
Please open a JIRA issue for changing the defaults. There was a similar issue about changing defaults for the Git plugin (JENKINS-33482). If we find a viable way to warn users about changing defaults (e.g. a warning in the log, failing the seed job, etc), we can change the defaults. |
Sure, no problem: JENKINS-33554 |
The default values for the thresholds are empty strings in the GUI and not zeros. Zeros have a different meaning, they enforce having 0 failed tests, while an empty string means "do not check this threshold".