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

Converting remaining anonymous Callable and FileCallable implementations to named classes #3437

Merged
merged 2 commits into from Jun 1, 2018

Conversation

2 participants
@jglick
Member

jglick commented May 11, 2018

Trying to take care of a bunch of log warnings in bulk.

@jglick jglick requested a review from oleg-nenashev May 11, 2018

@oleg-nenashev

Was it an automatic refactoring?

IMHO some static classes could use stricter APIs. FilePath is likely safe (do not recall), but it would be better to just resolve methods to FilePath#remote where possible.

Will do a deeper review next week

private class UntarFrom extends SecureFileCallable<Void> {
private final TarCompression compression;
private final InputStream in;
UntarFrom(TarCompression compression, InputStream in) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 11, 2018

Member

Must be RemoteInputStream IMHO

This comment has been minimized.

@jglick

jglick May 11, 2018

Member

This is a private class. The caller passes a working stream.

private class SymlinkTo extends SecureFileCallable<Void> {
private final String target;
private final TaskListener listener;
SymlinkTo(String target, TaskListener listener) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 11, 2018

Member

Is TaskListener Remotable OOTB?

This comment has been minimized.

@jglick

jglick May 11, 2018

Member

Yes it is.

}
private class UnzipFrom extends SecureFileCallable<Void> {
private final InputStream in;
UnzipFrom(InputStream in) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 11, 2018

Member

RemoteInputStream

This comment has been minimized.

@jglick

jglick May 11, 2018

Member

Does not really matter.

private final ArchiverFactory factory;
private final OutputStream out;
private final DirScanner scanner;
Archive(ArchiverFactory factory, OutputStream out, DirScanner scanner) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 11, 2018

Member

RemoteOutputStream

This comment has been minimized.

@jglick

jglick May 11, 2018

Member

Not if channel == null.

private class Archive extends SecureFileCallable<Integer> {
private final ArchiverFactory factory;
private final OutputStream out;
private final DirScanner scanner;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 11, 2018

Member

It's a bad idea to serialize it over the channel IMHO. Add better APIs and deprecate the existing method?

This comment has been minimized.

@jglick

jglick May 11, 2018

Member

The DirScanner needs to be serialized by the nature of this API.

act(new RenameTo(target));
}
private class RenameTo extends SecureFileCallable<Void> {
private final FilePath target;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 11, 2018

Member

Seems like a bad conversion. I would rather to do the target.remoreconversion outside and use File as argument

This comment has been minimized.

@jglick

jglick May 11, 2018

Member

For purposes of this PR I am not touching @kohsuke’s logic. Whether creating(new File(target.remote)) should be replaced with target.creating(new File(target.remote)) I am not sure. If you care to propose such changes, no problem, but they should not buried in a simple extract-nested-class refactoring like this.

act(new MoveAllChildrenTo(target));
}
private class MoveAllChildrenTo extends SecureFileCallable<Void> {
private final FilePath target;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 11, 2018

Member

same as above?

This comment has been minimized.

@jglick

jglick May 11, 2018

Member

ditto

@@ -2157,6 +2273,21 @@ public Void invoke(File f, VirtualChannel channel) throws IOException {
target.chmod(mode());
target.setLastModifiedIfPossible(lastModified());
}
private class CopyToWithPermission extends SecureFileCallable<Void> {
private final FilePath target;
CopyToWithPermission(FilePath target) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 11, 2018

Member

same as above

This comment has been minimized.

@jglick

jglick May 11, 2018

Member

ditto

@jglick

This comment has been minimized.

Member

jglick commented May 11, 2018

Was it an automatic refactoring?

No, IDE-assisted on each class.

some static classes could use stricter APIs. FilePath is likely safe

It is.

it would be better to just resolve methods to FilePath#remote where possible

Of course that would be nice, if it were straightforward. Unfortunately, it is not, due to the file path filter handling. #3333 (review) gives a full explanation.

@oleg-nenashev oleg-nenashev self-requested a review May 13, 2018

@jglick jglick added the needs-review label Jun 1, 2018

@oleg-nenashev

All comments are about private classes, so it does not matter much. Feel free to ship it

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jun 1, 2018

@jglick could you please merge with master to retrigger CI?

@oleg-nenashev oleg-nenashev merged commit 63fa178 into jenkinsci:master Jun 1, 2018

2 checks passed

continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@jglick jglick deleted the jglick:no-anonymous-serialization branch Jun 1, 2018

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