Skip to content
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

Cleanup #367

Merged
merged 6 commits into from
Feb 6, 2020
Merged

Cleanup #367

merged 6 commits into from
Feb 6, 2020

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Jan 22, 2020

While working on jenkinsci/jenkins#4407 (comment), I made a mistake in not recognizing that the default withMode is different between Channel(...) and ChannelBuilder(...).

I'm not absolutely certain about my comments, but if they're correct, it would help the next person like me perform a similar migration.

@@ -1865,7 +1916,7 @@ public static void dumpDiagnosticsForAll(@Nonnull PrintWriter w) {
* Notification that {@link Command#readFrom} has succeeded.
* @param cmd the resulting command
* @param blockSize the serialized size of the command
* @see CommandListener
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IntelliJ and everything else, myself included, were absolutely confused about this reference.

@@ -57,14 +57,14 @@ public ReaderThread(CommandReceiver receiver) {
public void run() {
final String name =channel.getName();
try {
while(!channel.isInClosed()) {
while (!channel.isInClosed()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These whitespace changes were enforced by IntelliJ when I made an unrelated debugging change (which is not included in this PR).

@@ -1618,7 +1665,9 @@ public OutputStream getUnderlyingOutput() {
* The remote port that the connection will be forwarded to.
* @return
* Created {@link PortForwarder}
* @deprecated as of 3.39
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PortForwarder class was Deprecated in #361, but no one bubbled up the deprecation within this module.
@jvz @jeffret-b

.withRestricted(restricted), transport);
.withArbitraryCallableAllowed(!restricted)
.withRemoteClassLoadingAllowed(!restricted)
, transport);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This style is used elsewhere within this file.

* @since 2.13
*/
@Deprecated
public Channel(String name, ExecutorService exec, CommandTransport transport, boolean restricted, ClassLoader base) throws IOException {
this(new ChannelBuilder(name,exec)
.withBaseLoader(base)
.withRestricted(restricted), transport);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

withRestricted is deprecated.

@jsoref jsoref force-pushed the cleanup branch 2 times, most recently from 669e7bf to a77585a Compare January 23, 2020 01:09
@jsoref
Copy link
Contributor Author

jsoref commented Jan 23, 2020

I'm not sure why Jenkins doesn't like my PRs of late. The changes here are really trivial. They should pass tests. I'm going to force push a couple of times until I have a sense of what's going on. Please don't review until I've figured out what's going on.

@jsoref
Copy link
Contributor Author

jsoref commented Jan 23, 2020

I'm done pushing to this PR. There was nothing wrong w/ any of my commits, the CI was flaky.

@jeffret-b
Copy link
Contributor

Thanks for the PRs. It will probably be a few days before I can get to them. Anyone else is also welcome to review.

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.

Looks good. Thanks for contributing and helping keep things cleaned up!

@jeffret-b jeffret-b added chore For changelog: A maintenance chore with no functional changes ready-to-merge labels Jan 27, 2020
@jeffret-b jeffret-b merged commit 2bea37f into jenkinsci:master Feb 6, 2020
@jsoref jsoref deleted the cleanup branch February 6, 2020 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore For changelog: A maintenance chore with no functional changes ready-to-merge
Projects
None yet
2 participants