Skip to content

[JENKINS-33596] Delete more of UDPBroadcastThread & DNSMultiCast#5720

Merged
timja merged 9 commits intojenkinsci:masterfrom
jglick:removeMulticast-redux
Sep 28, 2021
Merged

[JENKINS-33596] Delete more of UDPBroadcastThread & DNSMultiCast#5720
timja merged 9 commits intojenkinsci:masterfrom
jglick:removeMulticast-redux

Conversation

@jglick
Copy link
Member

@jglick jglick commented Sep 10, 2021

Follows up #4460. See JENKINS-33596. These classes have been stubs for over a year and a half; it is just confusing to leave them in the source tree.

Proposed changelog entries

  • Removing long-unused class UDPBroadcastThread, and deleting more of DNSMultiCast.

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@jglick jglick marked this pull request as draft September 10, 2021 16:30
@jglick jglick marked this pull request as ready for review September 10, 2021 16:54
@jglick
Copy link
Member Author

jglick commented Sep 10, 2021

Note that linked PRs mean that you will need to update tools to work with the new jenkins.version; you should be able to update tools anyway, though if you have a very old jenkins.version (< 2.220) you may see some delays and/or warnings.

@jglick
Copy link
Member Author

jglick commented Sep 10, 2021

Hmm, it occurs to me this could cause problems for people running PCT across a lot of plugins with older POMs. Maybe I need to keep just DNSMultiCast.disabled around to avoid linkage errors.

@jglick jglick marked this pull request as draft September 10, 2021 19:57
@jglick jglick changed the title [JENKINS-33596] Fully delete UDPBroadcastThread & DNSMultiCast [JENKINS-33596] Delete more of UDPBroadcastThread & DNSMultiCast Sep 10, 2021
@jglick jglick marked this pull request as ready for review September 10, 2021 20:01
Copy link
Contributor

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of using these old things, but I do know from experience there can be some odd dependencies on the existence of some of these pieces. While I'd like to get rid of it all, a more cautious excision as this has evolved into is more prudent.

@jglick
Copy link
Member Author

jglick commented Sep 13, 2021

a more cautious excision as this has evolved into is more prudent

Not sure I follow what you are saying. You approved this PR; do you have reservations?

@jeffret-b
Copy link
Contributor

No reservations. And I think it's safer to leave a little bit of it behind like you ended up doing.

@jglick
Copy link
Member Author

jglick commented Sep 13, 2021

To be clear, we cannot safely delete DNSMultiCast now; we could only delete it after any plugin which you might wish to run PCT against has updated to a new POM.

<groupId>${project.groupId}</groupId>
<artifactId>jenkins-test-harness</artifactId>
<version>1589.vc23fca066d5c</version>
<version>1626.v46b0925e70db</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the test failures related to updating this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any idea why dependabot isn't updating this?

updater | INFO <job_213087289> Checking if org.jenkins-ci.main:jenkins-test-harness 1589.vc23fca066d5c needs updating
  proxy | 2021/09/26 19:10:19 [146] GET https://repo.jenkins-ci.org:443/public/org/jenkins-ci/jenkins/1.67/jenkins-1.67.pom
  proxy | 2021/09/26 19:10:19 [146] 200 https://repo.jenkins-ci.org:443/public/org/jenkins-ci/jenkins/1.67/jenkins-1.67.pom
  proxy | 2021/09/26 19:10:19 [148] GET https://repo.jenkins-ci.org:443/public/org/jenkins-ci/main/jenkins-test-harness/maven-metadata.xml
  proxy | 2021/09/26 19:10:19 [148] 200 https://repo.jenkins-ci.org:443/public/org/jenkins-ci/main/jenkins-test-harness/maven-metadata.xml
  proxy | 2021/09/26 19:10:19 [150] GET https://repo.maven.apache.org:443/maven2/org/jenkins-ci/main/jenkins-test-harness/maven-metadata.xml
  proxy | 2021/09/26 19:10:19 [150] 404 https://repo.maven.apache.org:443/maven2/org/jenkins-ci/main/jenkins-test-harness/maven-metadata.xml
  proxy | 2021/09/26 19:10:19 [152] HEAD https://repo.jenkins-ci.org:443/public/org/jenkins-ci/main/jenkins-test-harness/1589.vc23fca066d5c/jenkins-test-harness-1589.vc23fca066d5c.jar
  proxy | 2021/09/26 19:10:19 [152] 200 https://repo.jenkins-ci.org:443/public/org/jenkins-ci/main/jenkins-test-harness/1589.vc23fca066d5c/jenkins-test-harness-1589.vc23fca066d5c.jar
updater | INFO <job_213087289> Latest version is 1589.vc23fca066d5c

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea offhand.

*
* @author Kohsuke Kawaguchi
* @deprecated No longer does anything.
* @deprecated No longer does anything. Only here to prevent errors from old versions of tools like {@code JenkinsRule}.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR tests still run with the older JenkinsRule

diff --git test/pom.xml test/pom.xml
index 1527fbcdcc..38d5934b15 100644
--- test/pom.xml
+++ test/pom.xml
@@ -81,7 +81,7 @@ THE SOFTWARE.
     <dependency>
       <groupId>${project.groupId}</groupId>
       <artifactId>jenkins-test-harness</artifactId>
-      <version>1626.v46b0925e70db</version>
+      <version>1589.vc23fca066d5c</version>
       <scope>test</scope>
       <exclusions>
         <exclusion>

@res0nance res0nance added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 21, 2021
@timja timja removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 26, 2021
@timja timja added skip-changelog Should not be shown in the changelog unresolved-merge-conflict There is a merge conflict with the target branch. labels Sep 26, 2021
@jglick jglick removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 27, 2021
@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 27, 2021
@timja
Copy link
Member

timja commented Sep 27, 2021

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants