-
Notifications
You must be signed in to change notification settings - Fork 168
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
[JENKINS-44266, JENKINS-36654] Fix PCT for 2.46.2 #103
Conversation
…st newer core versions (PCT).
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
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 seems suspicious. There is no mention of subversion
(or Subversion
or svn
) in the repository. So why do tests need this dependency?
When filing issues like JENKINS-44266 it would be appreciated if you would mention what the actual test failures are. Otherwise it is very difficult for reviewers to understand why you are proposing a change.
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.
🐛 until the issue gets clarified in JIRA. It is a very bad practice to report issues without explaining them (repro steps, stacktrace, link to failing test, etc.). Does not add much value to the Knowledge Base
@jglick @oleg-nenashev it's the usual issue with transitive test dependencies. I have improved the JIRA issue with the actual error message. |
So does jenkinsci/plugin-compat-tester#28 fix it? |
@jglick no, that would avoid a transitive optional dependency to be added to the POM, but this is failing even without modifying the POM (check that the command just changes the core version). |
I thought I saw that this plugin was not in the |
@oleg-nenashev @jglick do you think this need further work? |
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 suspect what you actually wanted to do was update the parent POM so as to pick up jenkinsci/jenkins-test-harness#54. But this seems harmless anyway.
@oleg-nenashev could you review this again? |
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.
Subversion plugin 2.5.2 requires Jenkins Core 1.568: https://github.com/jenkinsci/subversion-plugin/blob/subversion-2.5.2/pom.xml#L31
This plugin declares compatibility with 1.565. It means that all default testing will be effectively running with possibly unstable configuration.
As a maintainer, I suggest bumping the core dependency to 1.609.3 as a dependency resolution measure. I would also recommend to pick the new parent pom (2.27+) with JTH 2.22 as @jglick suggests. Probably there will be no need in such hack after that.
If you have no time for it, I will take a look later
@oleg-nenashev I will look at it, don't worry. |
@oleg-nenashev done and yes, adding subversion is not necessary now. |
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.
Some changes are really unrelated, but it is a good refactoring anyway. I will need to check the dependencies before merging.
Ideally @Bug
annotations should be replaced by @Issue
🐛 for removing repositories from pom.xml
pom.xml
Outdated
@@ -47,43 +47,11 @@ | |||
</license> | |||
</licenses> | |||
|
|||
<repositories> |
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.
🐛 In such case it won't be able to retrieve Parent pom if it is missing in the local cache.
OTOH you can simplify it to
<repositories>
<repository>
<id>repo.jenkins-ci.org</id>
<url>http://repo.jenkins-ci.org/public/</url>
</repository>
</repositories>
<pluginRepositories>
<pluginRepository>
<id>repo.jenkins-ci.org</id>
<url>http://repo.jenkins-ci.org/public/</url>
</pluginRepository>
</pluginRepositories>
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.
Well, use https
but otherwise yes that is the standard blurb.
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.
copypasted from the wrong plugin :(
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.
My bad, I thought this was already included in the parent POM.
@oleg-nenashev I have added back the "repositories" tag and replaced |
@@ -22,7 +21,7 @@ | |||
@Rule | |||
public JenkinsRule j = new JenkinsRule(); | |||
|
|||
@Bug(7972) | |||
@Issue("7972") |
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.
Please use full id, e.g. "JENKINS-7972"
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.
Done.
It also fixes https://issues.jenkins-ci.org/browse/JENKINS-36654 from what I see |
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.
🐝 and @reviewbybees done
Merging. Will see if I can release it today |
JENKINS-44266
@reviewbybees @oleg-nenashev @kwhetstone