Merge pull request #36880 from vutny/cp-get-salt-url

cp.get_url: fix `dest=None` behaviour with `salt://` URL

salt/fileclient.py

@@ -553,11 +553,20 @@ class Client(object):
                 raise CommandExecutionError(
                     'Path {0!r} is not absolute'.format(url_data.path)
                 )
+            if dest is None:
+                with salt.utils.fopen(url_data.path, 'r') as fp_:
+                    data = fp_.read()
+                return data
             return url_data.path
 
         if url_data.scheme == 'salt':
-            return self.get_file(
+            result = self.get_file(url, dest, makedirs, saltenv, cachedir=cachedir)
-                url, dest, makedirs, saltenv, cachedir=cachedir)
+            if result and dest is None:
+                with salt.utils.fopen(result, 'r') as fp_:
+                    data = fp_.read()
+                return data
+            return result
+
         if dest:
             destdir = os.path.dirname(dest)
             if not os.path.isdir(destdir):

salt/modules/cp.py

@@ -307,19 +307,42 @@ def get_dir(path, dest, saltenv='base', template=None, gzip=None, env=None, **kw
     return _client().get_dir(path, dest, saltenv, gzip)
 
 
-def get_url(path, dest, saltenv='base', env=None):
+def get_url(path, dest='', saltenv='base', env=None):
     '''
-    Used to get a single file from a URL.
+    Used to get a single file from a URL
 
-    The default behaviuor is to write the fetched file to the given
+    path
-    destination path. To simply return the text contents instead, set destination to
+        A URL to download a file from. Supported URL schemes are: ``salt://``,
-    None.
+        ``http://``, ``https://``, ``ftp://``, ``s3://``, ``swift://`` and
+        ``file://`` (local filesystem). If no scheme was specified, this is
+        equivalent of using ``file://``.
+        If a ``file://`` URL is given, the function just returns absolute path
+        to that file on a local filesystem.
+        The function returns ``False`` if Salt was unable to fetch a file from
+        a ``salt://`` URL.
+
+    dest
+        The default behaviour is to write the fetched file to the given
+        destination path. If this parameter is omitted or set as empty string
+        (``''``), the function places the remote file on the local filesystem
+        inside the Minion cache directory and returns the path to that file.
+
+        .. note::
+
+            To simply return the file contents instead, set destination to
+            ``None``. This works with ``salt://``, ``http://``, ``https://``
+            and ``file://`` URLs. The files fetched by ``http://`` and
+            ``https://`` will not be cached.
+
+    saltenv : base
+        Salt fileserver envrionment from which to retrieve the file. Ignored if
+        ``path`` is not a ``salt://`` URL.
 
     CLI Example:
 
     .. code-block:: bash
 
-        salt '*' cp.get_url salt://my/file /tmp/mine
+        salt '*' cp.get_url salt://my/file /tmp/this_file_is_mine
         salt '*' cp.get_url http://www.slashdot.org /tmp/index.html
     '''
     if env is not None:
@@ -331,10 +354,17 @@ def get_url(path, dest, saltenv='base', env=None):
         # Backwards compatibility
         saltenv = env
 
-    if dest:
+    if isinstance(dest, six.string_types):
-        return _client().get_url(path, dest, False, saltenv)
+        result = _client().get_url(path, dest, False, saltenv)
     else:
-        return _client().get_url(path, None, False, saltenv, no_cache=True)
+        result = _client().get_url(path, None, False, saltenv, no_cache=True)
+    if not result:
+        log.error(
+            'Unable to fetch file {0!r} from saltenv {1!r}.'.format(
+                path, saltenv
+            )
+        )
+    return result
 
 
 def get_file_str(path, saltenv='base', env=None):

tests/integration/modules/cp.py

@@ -146,31 +146,71 @@ class CPModuleTest(integration.ModuleCase):
 
     def test_get_url(self):
         '''
-        cp.get_url with salt:// source
+        cp.get_url with salt:// source given
         '''
         tgt = os.path.join(integration.TMP, 'scene33')
         self.run_function(
-                'cp.get_url',
+            'cp.get_url',
-                [
+            [
-                    'salt://grail/scene33',
+                'salt://grail/scene33',
-                    tgt,
+                tgt,
-                ])
+            ])
         with salt.utils.fopen(tgt, 'r') as scene:
             data = scene.read()
             self.assertIn('KNIGHT:  They\'re nervous, sire.', data)
             self.assertNotIn('bacon', data)
 
+    def test_get_url_dest_empty(self):
+        '''
+        cp.get_url with salt:// source given and destination omitted.
+        '''
+        ret = self.run_function(
+            'cp.get_url',
+            [
+                'salt://grail/scene33',
+            ])
+        with salt.utils.fopen(ret, 'r') as scene:
+            data = scene.read()
+            self.assertIn('KNIGHT:  They\'re nervous, sire.', data)
+            self.assertNotIn('bacon', data)
+
+    def test_get_url_no_dest(self):
+        '''
+        cp.get_url with salt:// source given and destination set as None
+        '''
+        tgt = None
+        ret = self.run_function(
+            'cp.get_url',
+            [
+                'salt://grail/scene33',
+                tgt,
+            ])
+        self.assertIn('KNIGHT:  They\'re nervous, sire.', ret)
+
+    def test_get_url_nonexistent_source(self):
+        '''
+        cp.get_url with nonexistent salt:// source given
+        '''
+        tgt = None
+        ret = self.run_function(
+            'cp.get_url',
+            [
+                'salt://grail/nonexistent_scene',
+                tgt,
+            ])
+        self.assertEqual(ret, False)
+
     def test_get_url_https(self):
         '''
-        cp.get_url with https:// source
+        cp.get_url with https:// source given
         '''
         tgt = os.path.join(integration.TMP, 'test_get_url_https')
         self.run_function(
-                'cp.get_url',
+            'cp.get_url',
-                [
+            [
-                    'https://repo.saltstack.com/index.html',
+                'https://repo.saltstack.com/index.html',
-                    tgt,
+                tgt,
-                ])
+            ])
         with salt.utils.fopen(tgt, 'r') as instructions:
             data = instructions.read()
             self.assertIn('Bootstrap', data)
@@ -178,6 +218,70 @@ class CPModuleTest(integration.ModuleCase):
             self.assertIn('Windows', data)
             self.assertNotIn('AYBABTU', data)
 
+    def test_get_url_https_dest_empty(self):
+        '''
+        cp.get_url with https:// source given and destination omitted.
+        '''
+        ret = self.run_function(
+            'cp.get_url',
+            [
+                'https://repo.saltstack.com/index.html',
+            ])
+        with salt.utils.fopen(ret, 'r') as instructions:
+            data = instructions.read()
+            self.assertIn('Bootstrap', data)
+            self.assertIn('Debian', data)
+            self.assertIn('Windows', data)
+            self.assertNotIn('AYBABTU', data)
+
+    def test_get_url_https_no_dest(self):
+        '''
+        cp.get_url with https:// source given and destination set as None
+        '''
+        tgt = None
+        ret = self.run_function(
+            'cp.get_url',
+            [
+                'https://repo.saltstack.com/index.html',
+                tgt,
+            ])
+        self.assertIn('Bootstrap', ret)
+        self.assertIn('Debian', ret)
+        self.assertIn('Windows', ret)
+        self.assertNotIn('AYBABTU', ret)
+
+    def test_get_url_file(self):
+        '''
+        cp.get_url with file:// source given
+        '''
+        tgt = ''
+        src = os.path.join('file://', integration.FILES, 'file/base/file.big')
+        ret = self.run_function(
+            'cp.get_url',
+            [
+                src,
+                tgt,
+            ])
+        with salt.utils.fopen(ret, 'r') as scene:
+            data = scene.read()
+            self.assertIn('KNIGHT:  They\'re nervous, sire.', data)
+            self.assertNotIn('bacon', data)
+
+    def test_get_url_file_no_dest(self):
+        '''
+        cp.get_url with file:// source given and destination set as None
+        '''
+        tgt = None
+        src = os.path.join('file://', integration.FILES, 'file/base/file.big')
+        ret = self.run_function(
+            'cp.get_url',
+            [
+                src,
+                tgt,
+            ])
+        self.assertIn('KNIGHT:  They\'re nervous, sire.', ret)
+        self.assertNotIn('bacon', ret)
+
     def test_cache_file(self):
         '''
         cp.cache_file