-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove custom XPath logic from PluginRemoting
#442
Conversation
// Trimming url | ||
transformedConnectionUrl = transformedConnectionUrl.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever
// Generally, when connectionUrl is empty, is implies it is declared in a parent pom | ||
// => Only possibility is to deduct github repository from artifactId (crossing fingers it | ||
// is not a bizarre repository url...) | ||
String oldUrl = transformedConnectionUrl; | ||
if (transformedConnectionUrl.isEmpty()) { | ||
transformedConnectionUrl = | ||
"scm:git:git://github.com/jenkinsci/" | ||
+ pomData.artifactId.replaceAll("jenkins", "") | ||
+ "-plugin.git"; | ||
if (!oldUrl.equals(transformedConnectionUrl)) { | ||
LOGGER.log( | ||
Level.WARNING, | ||
"project.scm.connectionUrl is not present in plugin's pom .. isn't it" | ||
+ " residing somewhere on a parent pom ?"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is actually a problem the solution is to update the parent POM to read <scm child.scm.connection.inherit.append.path="false" child.scm.developerConnection.inherit.append.path="false" child.scm.url.inherit.append.path="false">
per our archetype.
// Java.net SVN migration | ||
oldUrl = transformedConnectionUrl; | ||
transformedConnectionUrl = | ||
transformedConnectionUrl.replaceAll( | ||
"(svn|hudson)\\.dev\\.java\\.net/svn/hudson/", | ||
"svn.java.net/svn/hudson~svn/"); | ||
if (!oldUrl.equals(transformedConnectionUrl)) { | ||
LOGGER.log( | ||
Level.WARNING, | ||
"project.scm.connectionUrl is pointing to svn.dev.java.net/svn/hudson/ instead" | ||
+ " of svn.java.net/svn/hudson~svn/"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This migration happened a decade ago.
// ${project.artifactId} | ||
transformedConnectionUrl = | ||
transformedConnectionUrl.replaceAll( | ||
"\\$\\{project\\.artifactId\\}", pomData.artifactId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen a POM like this in years and Maven 4 actually won't accept it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the repository and pluginsRepository section - not for scm? (not that it matters here - but if you we are no longer able to use properties in an SCM connection there are a lot of plugins that need to be fixed for maven 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw it at least for dependencies.dependency.groupId
in JENKINS-70478.
// Convert things like | ||
// scm:git:git://git@github.com:jenkinsci/dockerhub-notification-plugin.git | ||
oldUrl = transformedConnectionUrl; | ||
transformedConnectionUrl = | ||
transformedConnectionUrl.replaceAll( | ||
"scm:git:git://git@github\\.com:jenkinsci", | ||
"scm:git:git://github.com/jenkinsci"); | ||
if (!oldUrl.equals(transformedConnectionUrl)) { | ||
LOGGER.log( | ||
Level.WARNING, | ||
"project.scm.connectionUrl should should be accessed in read-only mode (with" | ||
+ " git:// protocol)"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have spent months fixing up cases like this and I think most of the fixes have been released. If not then they should be, there is no reason to keep working around this in higher-level code.
oldUrl = transformedConnectionUrl; | ||
transformedConnectionUrl = | ||
transformedConnectionUrl.replaceAll("://github\\.com[^/]", "://github.com/"); | ||
if (!oldUrl.equals(transformedConnectionUrl)) { | ||
LOGGER.log( | ||
Level.WARNING, | ||
"project.scm.connectionUrl should have a '/' after the github.com url"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again why not just fix the root cause of the problem.
"project.scm.connectionUrl should not reference hudson project anymore (no" | ||
+ " plugin repository there))"); | ||
} catch (IOException e) { | ||
throw new UncheckedIOException(e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hudson has long been dead.
model.getParent().getArtifactId(), | ||
model.getParent().getVersion()); | ||
} else { | ||
parent = null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd wager this was fixed years ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ok pending downstream testing
// ${project.artifactId} | ||
transformedConnectionUrl = | ||
transformedConnectionUrl.replaceAll( | ||
"\\$\\{project\\.artifactId\\}", pomData.artifactId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the repository and pluginsRepository section - not for scm? (not that it matters here - but if you we are no longer able to use properties in an SCM connection there are a lot of plugins that need to be fixed for maven 4.
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.
Fixes #426 by replacing all the custom XPath logic in
PluginRemoting
with a few lines to delegate toMavenXpp3Reader
which is already on our class path and already knows how to parse Mavenpom.xml
files. To test this I ran the fulljenkinsci/bom
test suite.