Add some path checks around user provided paths

[kyleknap]

This mainly applies to the existence of user provided local paths. If the transfer is an upload and that local path does not exist, this will cause the command to fail instead of skipping. I realize this will cause a change in status code, but I think a hard fail is the correct behavior. Skipping on a specific path that the user specified should not be happening.

Also, If the transfer is a syncing download and the local path does not existent, there should not be warnings.

All integration test pass as well.

Fixes #1082, #1564

cc @jamesls @mtdowling @rayluo @JordonPhillips

[kyleknap]

I feel that this was sort of hacky when I wrote it as I am relying on the fact in the code when I instantiate the FileGenerator for the reference files in the destination, the operation_name is set to ''. I want some feedback on this part.

The other option I was thinking of was to introduce a new argument to the constructor that specifies the transfer direction. The reason I did not do it was because it would bloat the constructor and may require a bunch of updates to the tests. Note I am not too concerned about coupling to the command being run here because the FileGenerator already requires an operation name.

[jamesls]

Doesn't it seem simpler to just, much higher up the stack before doing an upload, validate that the local source exists? We should be able to do it before even getting into any of the command architecture code paths.

[kyleknap]

I think it was because I was concerned with follow_symlinks check right above it. But now that I think about it, I do not think that should be a concern because the if statment is only hit if the --no-follow_symlinks flag is set. If this is set, it will still hit the previous warning about a file not existing because the file will not be a symlink as it does not exist.

The other issue that I was concerned is with regards to #1082. If I add the logic higher up in the stack. The only way to fix this issue is to completely short circuit the reverse filegenerator, which I am not sure how easy it is for syncing. Maybe make a no op filegenerator but that seems messy to me.

I am fine with the first part, but with the second part, I do not have a great idea on how to get around it. Do you have any ideas on how to do that? I could hoist the logic to the call() method of the FileGenerator to prevent bloating the rest of the methods. Is that what you had in mind?

[jamesls]

@kyleknap I'm not following how these is a problem. Is your first concern we'd have to duplicate the symlink checking logic somewhere in CommandParams?

Also I'm not quite sure I understand the second point, maybe because I don't know the root cause. Why do we need to possibly short circuit the reverse filegenerator?

[kyleknap]

No the first paragraph is no longer an issue. I was just explaining (a little bit too my self as well) a possible issue, but it ended up not being an issue.

The second one is an issue because in order to avoid modifying the should_ignore method and not throw the warning, I need to avoid calling the list_files because currently it will always check the user provided path (which may not exist, but it does not matter because we will still download it and create the appropriate directories) for a downloading sync. So by short circuit, I mean I need to either provide a new reverse file generator that does a no op in the subcommands.CommandArchitecture class or immediately return before the list_files happens to avoid checking whether destination local directory exists. I think I am leaning toward the later option. Does that make sense?

[jamesls]

I'm definitely missing something. Can we not just create the directory before doing any transfers? Let me give something more specific. Why doesn't something like this work for your second case?

# in subcommands.py
if dir_op and path_type == 's3local':
  if not os.path.exists(params['dest']):
    os.makedirs(params['dest'])

[kyleknap]

Yeah that would be simpler. The only thing is that it is kind of weird that we are making modifications to the file system there as opposed to in s3handler and fileinfo.

[kyleknap]

Alright I updated the PR. Integration tests still pass.

[jamesls]

CommandParameters already has has pre-existing validation of parameters. Was there a reason not to add this logic to that class?

[kyleknap]

No, I just had forgotten about that class. Had not looked at it for a long time, and it shown. There were methods in the class that we not even used and some tests were using internal methods...

I update the PR. Integration tests still pass.

[jamesls]