Pipeline local filesystem: x permission on folders

[remileduc]

• add the "execute" permission on folders so users can see what is inside.

closes #226

[remileduc]

note that here, the x permission is added to folders only

[jhsimpson]

Thanks for the PR! I am not familiar with that chmod syntax, is 'Da+x' the same as 'a+X'?

I think this is a good change.
👍

[sevein]

@remileduc, what's the purpose of adding --perms when combined with --chmod?

I'm also wondering if we should use --chmod=ug+rw,ug+x instead, we don't need to give permissions to others right? Perhaps easier to read in octal mode: --chmod=F660,D770.

[remileduc]

@jhsimpson you have an explanation of what the D stands for here: https://explainshell.com/explain?cmd=rsync+-p+--chmod%3D%2Brwx+-e+%22ssh+-i+userserver.pem%22+--copy-unsafe-links+-rz+user%40server%3A~%2F+%2Fdb_backups%2Fwww%2F
It does the same as (upper) X, but I wasn't aware of it ^^

@sevein you're right, --perms would only be useful for already existing files, which shouldn't be the case here.

[remileduc]

Here, we KNOW we write a folder, so we can put the x permission on it.

As far as I'm concerned, this whole function may be replaced by the following code (but maybe I missed some features here):

    def create_rsync_directory(self, destination_path, user, host):
        # we know we have a paswordless SSH connection
        path = os.path.dirname(destination_path)  # thus we keep the previous behavior where we need a trailing slash or the folder is not created
        cmd = ['ssh', '{}@{}'.format(user, host), '"mkdir -p --mode=a+rwx " + path']
        
        LOGGER.info("path creation command: %s", cmd)
        try:
            subprocess.check_call(cmd)
        except subprocess.CalledProcessError as e:
            LOGGER.warning("rsync path creation failed: %s", e)
            raise

[jhsimpson]

I don't think you missed anything the function you suggest looks good. Do you want to update your PR to include it?

[remileduc]

(updated the code so we give a path to mkdir ^^)

Here, I assume there's a passwordless SSH configuration between the storage and the dashboard, as required by your documentation. However, from what I saw, only RSYNC is used to communicate between these 2 servers. Thus, somebody could have setup RSYNC to use another protocol (rsh? rsyncd?). I'm not sure it is possible, but using directly SSH like this would break it.
However, your documentation states that it needs SSH to be configured, this may be enough. What do you think?

I wait for your answer before editing the PR...

[jrwdunham]

@remileduc I like the idea of using ssh and mkdir -p to create the rsync directory. However, the third line of the snippet above has an error and will include the string 'path' instead of the value of the path var. It should be something like cmd = ['ssh', '{}@{}'.format(user, host), '"mkdir -p --mode=a+rwx {}"'.format(path)].

Also, in the snippet above --mode=a+rwx is being used while different permissions (--chmod=ug=rwx,o=rx) are being used in the current state of the PR.

Finally, are you sure that you want to call path = os.path.dirname(destination_path)? If destination_path has no trailing slash, then this will return the parent directory, which is not what you want, I think. If we want to always have a trailing slash we should continue to join on the empty string:

>>> import os
>>> os.path.dirname('a/b/c/')
'a/b/c'
>>> os.path.dirname('a/b/c')
'a/b'
>>> os.path.join('a/b/c/', '')
'a/b/c/'
>>> os.path.join('a/b/c', '')
'a/b/c/'

[remileduc]

You're totally right! I did the snippet above very quickly to show the main idea...

Should I update the PR with the mkdir version?

[jrwdunham]

I think so. Your strategy is a lot cleaner than the current one which needlessly makes a separate rsync call for each ancestor directory.

[remileduc]

I've updated the permissions with @sevein comments.
Let me know if you want me to put the SSH version for the second method.

[jrwdunham]

Replaced by #281