JENKINS-15925 - Changed delimiter of securityGroups from space to semi-colon.

[garlandkr]

JENKINS-15925 - Changed delimiter of securityGroups from space to semi-colon.

https://issues.jenkins-ci.org/browse/JENKINS-15925

[buildhive]

Jenkins » ec2-plugin #35 SUCCESS This pull request looks good (what's this?)

[francisu]

I'm a bit concerned about this as this will break existing configurations that use a space; these configurations should automatically be converted to use a semicolon (there is code that does this sort of thing elsewhere in the plugin). At the moment, I can't recall if there is some Jenkins versioning scheme that can be used to know if an old configuration has been converted, but there might be. We always need to make sure any previous configuration will work without modification. If you can provide the necessary conversion code and hooks that would be very helpful. If you are stuck with this let me know and I will look into it and get help.

[garlandkr]

I was not aware of these built-in methods, if I have time I'll take a look and try to get it incorporated.

[francisu]

Hi, the code that calls this is in PluginImpl, and you can see an example of this in InstanceTypeConverter. We might want to introduce a version number in the configuration to handle cases like this, because you have no other way of knowing that the configuration has been converted. Let me know if you want some help with this.

Francis

[jenkinsadmin]

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

[johnnyshields]

@francisu is there an example of a plugin (this one or other) which converts the config when loading the new version? (I imagine it would also need a "rollback")

[francisu]

This one is a bit tricky. Unfortunately there is no version mechanism in the metadata, so the migration is typically done by looking at the data and seeing if it's "old" or "new". However that's not possible in this case. One suggestion I got was to simply create a new name for the property with the data, but I find that unacceptable. The semantics of the property is exactly the same, so there is no cause to rename it. The only think I can think of is to add some kind of version property for the plugin as a whole and use that to help control this sort of migration. But then it gets even more messy as often the XML configuration files are updated directly (and not through the UI) so no checking is possible.

This is why this one has been sitting around for so long.

[johnnyshields]

@francisu is there any way to display a warning message when you upgrade?

[johnnyshields]

Hey wait a minute, the existing code looks like it's comma-delimited...

(Arrays.asList(securityGroups.split("\\s*,\\s*")));

This regex splits on commas and removes leading/trailing whitespace.

For example, the string "foo bar , bar qux" will be split into "foo bar" and "baz qux"

So I think the existing behavior should work as-is (comma instead of semi-colon). If we want to have both comma and semicolon we can do "\\s*[,;]\\s*"

[francisu]

It would be something good to write a JUnit for I think :)

On Mon, May 9, 2016 at 6:01 AM, Johnny Shields [email protected] wrote:

Hey wait a minute, the existing code looks like it's comma-delimited...

(Arrays.asList(securityGroups.split("\s_,\s_")));

This regex splits on commas and removes leading/trailing whitespace.

For example, the string "foo bar , bar qux" will be split into "foo bar" and "baz qux"

So I think the existing behavior should work as-is.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/jenkinsci/ec2-plugin/pull/28#issuecomment-217857571

Cell +1 510 432 1589 (when in US) Cell +33 7 83 72 48 66 (when in Europe)

[johnnyshields]

Studying a bit more on the problem, according to the docs here: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-network-security.html

The following are allowed:

a-z, A-Z, 0-9, spaces, and ._-:/()#,@[]+=&;{}!$*

So, actually neither comma or semi-colon (or space) is a good idea.

So, if we want to permanently solve the problem, there are two options:

1. Choose a delimiter not on the list, such as pipe |. Do a one-time conversion from commas to pipes when upgrading from v1.32 or lower.
2. Make the field a list input (with an "add" button.) Again this would require a conversion from the old field.