Fix #1312 : Container name should not be generated from maven group Id #1321
Conversation
|
@nicolaferraro @hrishin : Could you please review whenever you get time? |
| return projectGroupId; | ||
| } | ||
| else { | ||
| return projectGroupId.replaceAll("[^a-zA-Z0-9]", ""); |
There was a problem hiding this comment.
the regex [^a-zA-Z0-9] would also replace - which is desired?
| return projectGroupId; | ||
| } | ||
| else { | ||
| return projectGroupId.replaceAll("[^a-zA-Z0-9-]", ""); |
There was a problem hiding this comment.
This replacement still does not ensure that a name does not start or end with a -. Ok, a groupId propably never starts with a minus, but maybe it could end with one ?
I would add something like .replace("^-*(.*)-*$","\\1") ...
There was a problem hiding this comment.
Sorry, please correct me if I'm wrong. I tested my regex with groupId ending with a -, I think it handles. Plus how does your regex trims . from groupId? Does replace work with regex? I tried your regex with replaceAll but it seems to be replacing everything with 1
There was a problem hiding this comment.
@rohanKanojia I mean to add the regexp I suggest in addition to yours of course (and it was not tested, therefore that something like ...)
projectGroupId.replaceAll("[^a-zA-Z0-9-]", "").replaceFirst("^-*(.*)-*$","\\1")
The idea is to remove leading and trailing - from the name as these are not allowed.
|
@hrishin if the fix is good enough for you, could you please approve the PR ? thx! |
| } | ||
| else { | ||
| return projectGroupId.replaceAll("[^a-zA-Z0-9-]", "") | ||
| .replaceFirst("^-", "\\1") |
There was a problem hiding this comment.
Are you sure that this works ? You have no capturing group, so what should \1 refer to ? Actually you could use .replaceAll("^-*","") if you want to do it two steps.
There was a problem hiding this comment.
I tested it on groupIds like io.fabric8-test-, io.fabric8-test- and -io.fabric8-test-, Sorry I'm not an expert of regexes so might be doing some mistake. We can replace - with any character in the desired container characters range, I just thought replacing it with 1. Since you also suggested this in previous comment.
I tried your regex also i.e projectGroupId.replaceAll("[^a-zA-Z0-9-]", "").replaceFirst("^-*(.*)-*$","\\1") but it seems to be replacing everything with 1
There was a problem hiding this comment.
No, that \1 references the content of the capturing group in the previous expression (...). Its not "1" literally ;). Let me suggest a working solution and push it to your branch ...
There was a problem hiding this comment.
If you are interested how this works, look for "regular expression capturing group"
| .name("test") | ||
| .build(); | ||
| String containerName = KubernetesResourceUtil.extractContainerName(project, imageConfiguration); | ||
| assertTrue(containerName.matches(KubernetesResourceUtil.CONTAINER_NAME_REGEX)); |
There was a problem hiding this comment.
assertj is already in classpath why not starting writing assertions using it?
|
Looks good, thanks a lot ! And sorry for the confusion ... |
#1312