Sort dependencies #802
Sort dependencies #802
Conversation
| @@ -19,7 +19,15 @@ def managedPluginDeps = managedDeps.collect {stripAllButGA(it)}.grep { ga -> | |||
| if (settings.activeProfiles.any {it ==~ /^2[.][0-9]+[.]x$/}) { | |||
| println 'Skipping managed plugin dep sort check on this old LTS line' | |||
| } else { | |||
| def sortedDeps = managedPluginDeps.toSorted() | |||
| def sortedDeps = managedPluginDeps.toSorted { a, b -> | |||
There was a problem hiding this comment.
What behavior is being changed here?
There was a problem hiding this comment.
Is this a review, or a Q&A session? I really don't enjoy code reviews that consist of questions without a clear objective.
There was a problem hiding this comment.
To be clear, I have no problem answering the question. But leaving reviews that consist entirely of questions (especially on PRs that have already been merged, when there is no obvious regression) sends mixed messages that I can't always understand. There is almost always some underlying motivation behind the question. When a review consists of a question without an approval, there is often an implicit request for change in the question. Reviewing a PR that has already been merged, without an approval, can possibly imply that the PR should not have been merged. Articulating the goal explicitly helps make the intent and action items more clear.
To answer the question directly, the behavior that is being changed is that instead of sorting by a string of ${groupId}:${artifactId}", we now sort by group ID first and then artifact ID. This matches the Maven recommendations in the POM Code Convention. You can see the results in the pom.xml` that has been resorted. In the long term I plan to add SortPom integration to sanitize POMs automatically for core components and plugins on an opt-in basis. I am experimenting with various conventions and ways of doing this to find a balance with tolerable risk. As I go through this learning process I am trying out SortPom on various repositories and checking in the results where I find them acceptable. So far I am finding dependency sorting by scope, group ID, and artifact ID to be the best option, so I am trying it out on various repositories where I think the risk of regression is tolerable.
There was a problem hiding this comment.
I was really trying to understand what this PR purported to change, and the motivation. The title just said it was sorting dependencies, but they were already sorted; it was not clear to me that the change is to switch some detail of the sorting algorithm.
For purposes of Jenkins plugin dependencies, my general preference is to sort by short name, which is almost always the same as artifactId, since that is what Jenkins itself pays attention to; the groupId is generally uninteresting (and sometimes some weird value predating @jenkinsci hosting). But I do not feel strongly about it. The important thing is to have some sort order enforced, which defends against certain problems at scale like merge conflicts.
There was a problem hiding this comment.
I was really trying to understand what this PR purported to change, and the motivation. The title just said it was sorting dependencies, but they were already sorted; it was not clear to me that the change is to switch some detail of the sorting algorithm.
Got it, though that wasn't clear to me from the fact that you used the "review" rather than "comment" feature. I will try and write more about the problem, solution, implementation details, etc. Not sure why I tend to write more in Review Board compared to GitHub; perhaps it's just the tiny size of the textarea that discourages me. Tools matter. I should just write my usual long change descriptions in a text editor and paste them into GitHub when done.
For purposes of Jenkins plugin dependencies, my general preference is to sort by short name, which is almost always the same as
artifactId, since that is what Jenkins itself pays attention to; thegroupIdis generally uninteresting (and sometimes some weird value predating @jenkinsci hosting). But I do not feel strongly about it. The important thing is to have some sort order enforced, which defends against certain problems at scale like merge conflicts.
Yeah, unfortunately SortPom (which seems to be the best candidate for us to use since it's the only one with Spotless integration) can't sort by short name, just group ID and artifact ID. So that's what I've been going with. The Maven POM Code Convention merely says that "grouping dependencies by topics (like groupId i.e. org.apache.maven) is a good practice."
There was a problem hiding this comment.
you used the "review" rather than "comment" feature
Ah OK. Just trying to point to a specific line for concreteness. Unfortunately GH does not let you specify a 👍 / 👎 / neutral “comment” gesture when “reviewing” a merged PR.
the tiny size of the textarea
Huh, I am able to resize it. On Chromium.
Sorts dependencies by group ID and then artifact ID.