allow interpolation of SCM urls #450
Conversation
ammend jenkinsci#442 to allow use of project.artifactId and artifactId as properties inside the SCM section testing a project with maven 4.0.0-alpha4 showed no warnings or errors about this use case, and we have many plugins that use it. Whilst fixing all the plugin is nobale - that would unessescarily hold up future improvements.
| if (original == null) { | ||
| return null; | ||
| } | ||
| return original.replace("${project.artifactId}", artifactId).replace("${artifactId}", artifactId); |
There was a problem hiding this comment.
whilst this could have used a regular expression to do a replacement in one - this is easier to test and any runtime overhead difference between the 2 would disapear into the timing of actually running tests.
There was a problem hiding this comment.
Ah I see, looks like I was a bit too aggressive in #442. The original code had:
transformedConnectionUrl.replaceAll(
"\\$\\{project\\.artifactId\\}", pomData.artifactId);so we probably only need to deal with project.artifactId rather than artifactId. There can't be too many of these if this is working in BOM: how hard would it be to just fix them up? Also are these mostly proprietary or open source plugins?
Also the method could be private.
There was a problem hiding this comment.
Package scope to allow testing the method.
Had tried a regexp so that really needed the test but switched from regexp and left the test.
Happy to simplify to just project.artifactId.
Looks to be about 30+ stopped triaging after seeing same failure repeated.
Given this is a simple change I would say once I have a pr to validate this we should merge and I can fixup plugins at leisure and then we can eventually revert this.
There was a problem hiding this comment.
Oh I forgot to commit the unit test 🤦
There was a problem hiding this comment.
Anything that has been incrementalified should be using flatten-maven-plugin which will interpolate these variables in the published POM so I am guessing this is probably a bigger issue in proprietary plugins.
There was a problem hiding this comment.
yup, most are proprietary, electricflow is the only one that jumps out from OSS land.
There was a problem hiding this comment.
probably only need to deal with project.artifactId rather than artifactId
Adapted in 817dafc
There was a problem hiding this comment.
electricflowis the only one that jumps out from OSS land
Which I filed a PR to fix two weeks ago: jenkinsci/electricflow-plugin#333
Never ceases to amaze me how long it takes for my PRs to be acknowledged and merged.
There was a problem hiding this comment.
Oh never mind, that PR wouldn't have fixed the issue because the plugin isn't incrementalized. I'll file a separate PR to incrementalify it.
There was a problem hiding this comment.
| if (original == null) { | ||
| return null; | ||
| } | ||
| return original.replace("${project.artifactId}", artifactId).replace("${artifactId}", artifactId); |
There was a problem hiding this comment.
Ah I see, looks like I was a bit too aggressive in #442. The original code had:
transformedConnectionUrl.replaceAll(
"\\$\\{project\\.artifactId\\}", pomData.artifactId);so we probably only need to deal with project.artifactId rather than artifactId. There can't be too many of these if this is working in BOM: how hard would it be to just fix them up? Also are these mostly proprietary or open source plugins?
Also the method could be private.
ammend #442 to allow use of project.artifactId and artifactId as properties inside the SCM section
testing a project with maven 4.0.0-alpha4 showed no warnings or errors about this use case, and we have many plugins that use it.
Whilst fixing all the plugin is nobale - that would unessescarily hold up future improvements.