Fix PluginWrapper bug and cleanup Javadoc placeholders#25984
Fix PluginWrapper bug and cleanup Javadoc placeholders#25984MarkEWaite merged 11 commits intojenkinsci:masterfrom
Conversation
- Fix inconsistent synchronization in Fingerprint.java on 'usages' field by synchronizing on 'this'. - Fix logic bug in PluginWrapper.java where setOptionalDependants passed incorrect field instead of parameter. - Replace Boolean.getBoolean with SystemProperties.getBoolean in Jenkins.java and JnlpSlaveRestarterInstaller.java for best practices. - Update placeholder version numbers (@deprecated and @SInCE) in StreamTaskListener, StreamBuildListener, and JNLPLauncher. Signed-off-by: shbhmexe <shubhushukla586@gmail.com>
MarkEWaite
left a comment
There was a problem hiding this comment.
This seems like a combination of multiple changes into a single pull request. Please limit the scope of changes in pull requests so that reviewers are more likely to review the change and so that they can be clearly described.
The change of argument for the setOptionalDependants method is correct. Since the method is unused, the change will likely have no effect, but the change is correct.
The updates of the Javadoc comments generally mention a version prior to the deprecation. We list the version that first deprecated the method. Since there are very few of these cases, it is probably not worth the effort to update the update-since-todo.py script that automates the replacement of TODO with a release date.
The change of synchronization in the Fingerprint class does not have enough justification for the change and it lacks testing. If that justification and testing are added, it should be a separate commit.
The change from Boolean.getBoolean() to SystemProperties.getBoolean does not have enough justification for the change. Specifically, there is no mention of a reason why the ServletContext init context should be consulted for the property in addition to checking the system properties. If that justification and sufficient testing are added, it should be a separate commit
core/src/main/java/jenkins/slaves/restarter/JnlpSlaveRestarterInstaller.java
Outdated
Show resolved
Hide resolved
Thank you for the feedback @MarkEWaite. I understand your concern about the PR scope. I will split this PR into separate, focused pull requests: Javadoc version updates - Fixing deprecated version placeholders |
Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
…Installer.java Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
|
In its current state, the changes in this pull request look reasonable, so long as the pull request description is updated to complete the pull request template and remove mention of things that are no longer being changed in this pull request. |
|
Thanks @MarkEWaite! I've updated the PR description to reflect only the current changes (PluginWrapper fix + Javadoc updates). Removed mention of Fingerprint and SystemProperties changes. |
??? |
|
Sorry, I forgot to update the PR title! The synchronization change was removed per earlier feedback. This PR now only contains the PluginWrapper fix and Javadoc version placeholder updates. Updated the title now. |
MarkEWaite
left a comment
There was a problem hiding this comment.
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.
/label ready-for-merge
This PR fixes a logic bug in an unused, deprecated method in
PluginWrapperand updates placeholder version numbers in Javadoc deprecation annotations.Testing done
mvn compile -pl core -q- PASSEDProposed changelog entries
PluginWrapper.setOptionalDependants()method.Proposed changelog category
/label skip-changelog
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.Desired reviewers
@jenkinsci/core-pr-reviewers