Fixing throttle based on parameter examination (Replaces PR#33) #38
Conversation
|
Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests. |
|
It's difficult to review for too many unrelated changes (whitespace changes, breaking lines, swapping lines). Please turn off the autoformatter of your IDE, update the commit, and let me see changes that you actually want to be merged. |
|
@ikedam - Sorry about the whitespace. Is there a code style template that you use to prevent standard code styles from being applied? |
|
I can't help my IDE cleaning up trailing spaces on end of lines. They don't need to be there. |
|
Also - I think as a suggesting that the project should adopt some sort of standard codestyle and format to that. I will put forward: http://code.google.com/p/google-styleguide/ Here is the Metanome info for applying google code style: |
|
Looks like the field is not persisted in my manual tests. I will have to take a look. |
|
I don't use Intellij and I can't help you about configurations.
|
|
I've tried to eliminate the white spaces in intellij/eclipse - it keeps putting them back. I am working on it, I am re-opening this for the time being for feedback. I was missing some of the original logic. It is all back in place and i have just tested it manually to ensure everything is persisted. |
|
Re-opening for comment. I am trying to deal with the whitespace and attempting to put back the extraneous spacing. |
|
I'll be honest, I haven't been deep into this plugin in a long time, so I'd love it if @oleg-nenashev could give it a look. =) |
|
@oleg-nenashev - can you approve/merge this? |
1 similar comment
|
@oleg-nenashev - can you approve/merge this? |
|
Would love to see this merged in as well! |
|
I am thinking of basically creating a new Jenkins plugin based on this branch. There are multiple people wanting this functionality. |
|
Eh, merging it - @oleg-nenashev, feel free to revert if you have issues. |
|
How do you get this into a release? Who can trigger a release? |
|
I'll get one out tomorrow. |
|
@abayer - were you able to get a release out today? |
|
Releasing now - should show up in the update center in a few hours. |
|
@abayer Thanks! |
|
@abayer - https://wiki.jenkins-ci.org/display/JENKINS/Throttle+Concurrent+Builds+Plugin this still shows 1.84. The latest release does not show up in update center. |
| @@ -57,6 +64,8 @@ public ThrottleJobProperty(Integer maxConcurrentPerNode, | |||
| List<String> categories, | |||
| boolean throttleEnabled, | |||
| String throttleOption, | |||
| boolean limitOneJobWithMatchingParams, | |||
There was a problem hiding this comment.
This change breaks the binary compatibility.
As well as previous ones, so no complains
|
Can we at least require that pull requests are squashed! This pull request is impossible to review for what it does, and the white space noise is incredible, how was this merged? |
|
@oleg-nenashev - you had over a month, possibly two to react. I needed the functionality and simply fixed the existing PR to work. By fixed, I simply took the existing PR as it was and fixed the things that were wrong in my opinion. I was working from a snapshot image, so having it merged was not the highest priority for me. I pinged you multiple times over an extensive period, with no reply. @abayer was a least responsive in the situation. I would like to understand what you think the additional burden is in a maintenance perspective? If you need help with it, I can pitch in if you like or not. You can turn on squashing by default now (I believe). Maybe that should be done? @Sauraus - If you are adverse to white space cleanup, someone should provide at a minimum a code style template for the plugin. IDE's react differently and some do the proper thing by cleaning up (removing trailing WS, blank lines etc). It is extremely hard to satisfy everyone in a situation where open source is present. A lot of whining makes it look highly unprofessional in my opinion. |
|
@balldarrens I am not against white space clean up, if it's done as a standalone commit, because doing it as part of a feature is begging for trouble. And just to be clear, the Jenkins community documentation on plugin development has this little gem in it PS. I fully support the feature and appreciate you taking the time to implement it and I am sure it will be useful too many including myself. |
|
@Sauraus - 47 days, basically how long this stood still without maintainer comments. Possibly longer, as the original PR was closed and re-opened. How long should one wait for PR feedback? That is sort of excessive no? The previous PR (#33) sat there for quite a while as well, even with requests for help solving the problems they encountered... No one chimed in. If you take those two time periods, the timeframe goes from 47 days to > 1/4 of a year (#33 opened Jan 8). That is pretty insane. Where was the maintainer during that period of time? I decided to help, where I could, in order to get the functionality needed/wanted. I wasn't threatening anything, the fact is that I would actually follow through on that if need be. If that is what it takes to get something into play (it shouldn't). I would always rather contribute and help than to do that. It shouldn't take that long to get constructive feedback. Maybe there is another entry in the myriad of docs on Jenkins regarding the acceptable wait period and I've missed that as well. The very fact that the statement exists regarding IDE formatting suggests to me that the whole plugin contribution environment needs to be enforced by checkstyle or another similar widely used mechanism which it seemingly does not. A mechanism as such would be pretty cut and dried. Fail checkstyle, fix it. No arguments. P.S I appreciate any feedback, especially if it is timely. I hope the changes are useful, as there are in my environment. I've been using it for almost two months with no issues. |
|
@balldarrens Here is the recommended process (documented somewhere on Wiki):
I'm receiving hundreds of GitHub notifications per day, so I may easily miss something (especially in the case of the family urgency). That's why I keep my e-mail public. |
|
@oleg-nenashev - I am not suggesting you dropped the ball. Don't take issue with other's pushing a release without your approval moving forward. If you can't be available for the plugin. Comments like it shouldn't have been released are not very constructive. Contact the contributor and ask them to fix it. If you are overloaded (personal life, work life etc.) let others fix and learn. Worse case revert and re-open the PR and make comments. P.S. Sorry to hear about a family emergency. Let's carry on, fix where we can and learn what we can from you regarding making more thoughtful changes. |
|
I tried this feature with a selection of parameters to compare for throttling and it looks like it is ignoring the different values of the parameters: I am not sure from look at the code: Are really the values of the parameters checked for differences? It looks like it is checking the name of the parameters? |
I can say I found the same behaviour: builds queued despite using different values for the parameters on the "watch" list. On the other hand, this plugin seems to be "abandonware". Is that the case? |
|
@jmnavarrol It is adoptaware ;) |
Thanks for the update. |
Please review and comment.
@oleg-nenashev
@jmozmoz
@abayer
@ikedam