Don't enforce upper case for variables. #6
Conversation
If you have software that expects lower case variable, then you are unable to use this plugin at all.
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
| @@ -190,7 +190,7 @@ private void checkPortNumbers(String ports) throws FormException { | |||
|
|
|||
| public Pool getPoolByName(String poolName) throws PoolNotDefinedException { | |||
| for (Pool p : pools) { | |||
| if (p.name.toUpperCase().equals(poolName.toUpperCase())) { | |||
| if (p.name.equals(poolName)) { | |||
There was a problem hiding this comment.
It's a breaking change BTW. I think the author just wanted to have a case-insensitive comparison, so I would use http://docs.oracle.com/javase/6/docs/api/java/lang/String.html#equalsIgnoreCase(java.lang.String)
There was a problem hiding this comment.
For case-sensitive system i will be unable to use two variables different in registry.
There was a problem hiding this comment.
But i can accept such limitation. Then comparison may really makes sense.
|
I don't agree with both comments. It may resolve your use-case, but it's a potentially breaking change. I don't use the plugin, so I cannot elaborate the real impact. @oldelvet WDYT? |
|
According to pom.xml @pepov @kohsuke @ramapulavarthi highlight. |
|
I only really use the plugin in conjunction with the android-emulator-plugin so do not have much experience with the use of port names. That said the existing version converts any name into an upper case name that gets stored in the job config.xml file. IMHO the import thing is that any existing configurations should not be broken by the change. My initial look at the code suggests that this is the case. Any existing variables will be set as upper case only and any conflicting port checks would be done using the upper case variable names. With the change in place lower/mixed case variables would then be treated as being from separate pools and this is where any breakage would potentially occur. I can see that this may be a problem for new/changed job configurations using existing variables - but any existing variables would be currently listed as being upper case so it would only be people who are used to setting up lots of port setups and not looking at/using the variables. I think it reasonable that we mitigate this by documenting the change in the release notes/popup help text. The only cases that I don't have a feel for are where automatic configuration generators may be in use and also any programmatic uses of the allocator from other plugins using the names (android emulator just uses port ranges so is not affected). |
If you have software that expects lower case variable, then you are unable to use this plugin at all.