Repository: saltstack/salt
Pull request: Bug fixes in Junos-related components
### What does this PR do?
Multiple bug fixes were done for Junos-related components in salt. 

### What issues does this PR fix or reference?
- Fixes https://github.com/Juniper/salt/issues/85, https://github.com/Juniper/salt/issues/86, https://github.com/Juniper/salt/issues/88, https://github.com/Juniper/salt/issues/89, https://github.com/Juniper/salt/issues/90, https://github.com/Juniper/salt/issues/93, https://github.com/Juniper/salt/issues/94, https://github.com/Juniper/salt/issues/96, https://github.com/Juniper/salt/issues/99, https://github.com/Juniper/salt/issues/100, https://github.com/Juniper/salt/issues/103, https://github.com/Juniper/salt/issues/104, https://github.com/Juniper/salt/issues/109, https://github.com/Juniper/salt/issues/110, https://github.com/Juniper/salt/issues/112, https://github.com/Juniper/salt/issues/113, https://github.com/Juniper/salt/issues/114, https://github.com/Juniper/salt/issues/117

### Tests written?
Yes

Problem statement:
install_config times out on longer commit checks
Using new timeoutdecorator still doesn't fix install_config for me when commit_check takes 30+ seconds as well. Looks like timeout is set temporarily for certain tasks but not the commit check.

The only way I can get longer device commits to work (that take +30 seconds to commit check as well) is to modify the device.py itself to hard code the timeout property to 300, then restarting the proxy-minion.

I would recommend testing anything timeout related with an SRX300. You can use a state with a configuration group of ~10 items (doesn't need to be enormous) and applying that group. You will want to make sure a commit check on the device itself takes longer than 30 seconds for what the state is trying to apply.

10 security address objects or 10 dhcp bindings should do the trick.

A better approach to this in general would be to start the proxy minion with an RPC timeout option to override the default of 30.
Something like:
"salt-proxy --proxyid=SLOWSRX -d -t 300"

If commit checks are timing out these might show up in install_config as unlockerrors or rpctimeouts from my testing.

edit: timeout support would make more sense in proxy data in the pillar
   timeout: 300   (would be amazing)

Hints from issue discussion:
@Zapotomous Could you please provide the following details.
* What branch of the repository are you currently using ??
Example :  
```
git branch
* 2018.3.3
  develop
  updated_develop
```
* What is the salt command you are trying to run ??
Example:
`salt 'device_name' junos.install_config 'salt://config.set' dev_timeout=300`
I pulled down the updated_develop branch again and can't reproduce the issue I was having before where dev_timeout would not work. I've tested passing dev_timeout to rollback and install_config several times with no issues now.

Thanks for opening a task for pillar option timeout.

Commit message: fix: salt.states.junos.rst:20:Pygments lexer name 'set' is not known

Patch to review:
diff --git a/requirements/static/darwin.in b/requirements/static/darwin.in
index 9e48f6151d9d..9a73a138ca4e 100644
--- a/requirements/static/darwin.in
+++ b/requirements/static/darwin.in
@@ -8,7 +8,7 @@ docker
 futures>=2.0; python_version < '3.0'
 jmespath
 jsonschema
-junos-eznc
+junos-eznc==2.4.0
 jxmlease
 keyring==5.7.1
 kubernetes<4.0
diff --git a/requirements/static/linux.in b/requirements/static/linux.in
index a12d551f9ed2..c7a4086905b6 100644
--- a/requirements/static/linux.in
+++ b/requirements/static/linux.in
@@ -12,7 +12,7 @@ GitPython
 hgtools
 jmespath
 jsonschema
-junos-eznc
+junos-eznc==2.4.0
 jxmlease
 kazoo
 keyring==5.7.1
diff --git a/salt/engines/junos_syslog.py b/salt/engines/junos_syslog.py
index a55b2b8dac71..f945fdf1b1f6 100644
--- a/salt/engines/junos_syslog.py
+++ b/salt/engines/junos_syslog.py
@@ -27,29 +27,29 @@
 9.   pid
 10.   raw (the raw event data forwarded from the device)
 
-The topic title can consist of any of the combination of above fields, but the
-topic has to start with ``jnpr/syslog``. Here are a couple example
-combinations:
+The topic title can consist of any of the combination of above fields,
+but the topic has to start with 'jnpr/syslog'.
+So, we can have different combinations:
 
-- jnpr/syslog/hostip/daemon/event
-- jnpr/syslog/daemon/severity
+ - jnpr/syslog/hostip/daemon/event
+ - jnpr/syslog/daemon/severity
 
 The corresponding dynamic topic sent on salt event bus would look something like:
 
-- jnpr/syslog/1.1.1.1/mgd/UI_COMMIT_COMPLETED
-- jnpr/syslog/sshd/7
+ - jnpr/syslog/1.1.1.1/mgd/UI_COMMIT_COMPLETED
+ - jnpr/syslog/sshd/7
 
-The default topic title is ``jnpr/syslog/hostname/event``.
+The default topic title is 'jnpr/syslog/hostname/event'.
 
-One can choose the type of data they want from the event bus. For example, if
-one wants only events pertaining to a particular daemon, this can be specified
-in the configuration file:
+The user can choose the type of data they wants of the event bus.
+Like, if one wants only events pertaining to a particular daemon, they can
+specify that in the configuration file:
 
 .. code-block:: yaml
 
     daemon: mgd
 
-One can even have a list of daemons:
+One can even have a list of daemons like:
 
 .. code-block:: yaml
 
@@ -72,15 +72,15 @@
 For junos_syslog engine to receive events, syslog must be set on the junos device.
 This can be done via following configuration:
 
-.. code-block:: text
+.. code-block:: bash
 
     set system syslog host <ip-of-the-salt-device> port 516 any any
 
 Below is a sample syslog event which is received from the junos device:
 
-.. code-block:: text
+.. code-block:: bash
 
-    <30>May 29 05:18:12 bng-ui-vm-9 mspd[1492]: No chassis configuration found
+    '<30>May 29 05:18:12 bng-ui-vm-9 mspd[1492]: No chassis configuration found'
 
 The source for parsing the syslog messages is taken from:
 https://gist.github.com/leandrosilva/3651640#file-xlog-py
@@ -154,7 +154,7 @@ def __init__(self):
         priority = Suppress("<") + ints + Suppress(">")
 
         # timestamp
-        month = Word(string.uppercase, string.lowercase, exact=3)
+        month = Word(string.ascii_uppercase, string.ascii_lowercase, exact=3)
         day = ints
         hour = Combine(ints + ":" + ints + ":" + ints)
 
@@ -319,7 +319,7 @@ def parseData(self, data, host, port, options):
                  if the event is to be sent on the bus.
 
         """
-        data = self.obj.parse(data)
+        data = self.obj.parse(data.decode())
         data["hostip"] = host
         log.debug(
             "Junos Syslog - received %s from %s, sent from port %s", data, host, port
diff --git a/salt/modules/junos.py b/salt/modules/junos.py
index 36c100878c3f..1b87f906ae3a 100644
--- a/salt/modules/junos.py
+++ b/salt/modules/junos.py
@@ -17,8 +17,11 @@
 # Import Python libraries
 from __future__ import absolute_import, print_function, unicode_literals
 
+import glob
+import json
 import logging
 import os
+import traceback
 from functools import wraps
 
 # Import Salt libs
@@ -26,6 +29,8 @@
 import salt.utils.files
 import salt.utils.json
 import salt.utils.stringutils
+import yaml
+from salt.exceptions import MinionError
 from salt.ext import six
 
 try:
@@ -45,6 +50,11 @@
     import jnpr.junos.utils
     import jnpr.junos.cfg
     import jxmlease
+    from jnpr.junos.factory.optable import OpTable
+    import jnpr.junos.op as tables_dir
+    from jnpr.junos.factory.factory_loader import FactoryLoader
+    import yamlordereddictloader
+    from jnpr.junos.exception import ConnectClosedError, LockError
 
     # pylint: enable=W0611
     HAS_JUNOS = True
@@ -71,8 +81,9 @@ def __virtual__():
     else:
         return (
             False,
-            "The junos module could not be loaded: "
-            "junos-eznc or jxmlease or proxy could not be loaded.",
+            "The junos or dependent module could not be loaded: "
+            "junos-eznc or jxmlease or or yamlordereddictloader or "
+            "proxy could not be loaded.",
         )
 
 
@@ -149,7 +160,7 @@ def facts():
 
 
 @timeoutDecorator
-def rpc(cmd=None, dest=None, **kwargs):
+def rpc(cmd=None, **kwargs):
     """
     This function executes the RPC provided as arguments on the junos device.
     The returned data can be stored in a file.
@@ -181,8 +192,8 @@ def rpc(cmd=None, dest=None, **kwargs):
 
     .. code-block:: bash
 
-        salt 'device' junos.rpc get_config /var/log/config.txt format=text filter='<configuration><system/></configuration>'
-        salt 'device' junos.rpc get-interface-information /home/user/interface.xml interface_name='lo0' terse=True
+        salt 'device' junos.rpc get_config dest=/var/log/config.txt format=text filter='<configuration><system/></configuration>'
+        salt 'device' junos.rpc get-interface-information dest=/home/user/interface.xml interface_name='lo0' terse=True
         salt 'device' junos.rpc get-chassis-inventory
     """
 
@@ -195,10 +206,6 @@ def rpc(cmd=None, dest=None, **kwargs):
         ret["out"] = False
         return ret
 
-    format_ = kwargs.pop("format", "xml")
-    if not format_:
-        format_ = "xml"
-
     op = dict()
     if "__pub_arg" in kwargs:
         if kwargs["__pub_arg"]:
@@ -211,10 +218,19 @@ def rpc(cmd=None, dest=None, **kwargs):
     else:
         op.update(kwargs)
 
+    format_ = op.pop("format", "xml")
+    dest = op.pop("dest", None)
+
     if cmd in ["get-config", "get_config"]:
         filter_reply = None
         if "filter" in op:
-            filter_reply = etree.XML(op["filter"])
+            try:
+                filter_reply = etree.XML(op["filter"])
+            except etree.XMLSyntaxError as ex:
+                ret["message"] = "Invalid filter: {0}".format(str(ex))
+                ret["out"] = False
+                return ret
+
             del op["filter"]
 
         op.update({"format": format_})
@@ -264,12 +280,12 @@ def set_hostname(hostname=None, **kwargs):
     hostname
         The name to be set
 
-    dev_timeout : 30
-        The NETCONF RPC timeout (in seconds)
-
     comment
         Provide a comment to the commit
 
+    dev_timeout : 30
+        The NETCONF RPC timeout (in seconds)
+
     confirm
       Provide time in minutes for commit confirmation. If this option is
       specified, the commit will be rolled back in the specified amount of time
@@ -332,6 +348,7 @@ def set_hostname(hostname=None, **kwargs):
         ret["out"] = False
         ret["message"] = "Successfully loaded host-name but pre-commit check failed."
         conn.cu.rollback()
+
     return ret
 
 
@@ -419,6 +436,7 @@ def commit(**kwargs):
         ret["out"] = False
         ret["message"] = "Pre-commit check failed."
         conn.cu.rollback()
+
     return ret
 
 
@@ -451,7 +469,7 @@ def rollback(**kwargs):
 
     .. code-block:: bash
 
-        salt 'device_name' junos.rollback 10
+        salt 'device_name' junos.rollback id=10
     """
     id_ = kwargs.pop("id", 0)
 
@@ -526,7 +544,7 @@ def diff(**kwargs):
 
     .. code-block:: bash
 
-        salt 'device_name' junos.diff 3
+        salt 'device_name' junos.diff id=3
     """
     kwargs = salt.utils.args.clean_kwargs(**kwargs)
     id_ = kwargs.pop("id", 0)
@@ -633,7 +651,6 @@ def cli(command=None, **kwargs):
     .. code-block:: bash
 
         salt 'device_name' junos.cli 'show system commit'
-        salt 'device_name' junos.cli 'show version' dev_timeout=40
         salt 'device_name' junos.cli 'show system alarms' format=xml dest=/home/user/cli_output.txt
     """
     conn = __proxy__["junos.conn"]()
@@ -670,8 +687,13 @@ def cli(command=None, **kwargs):
         ret["message"] = jxmlease.parse(result)
 
     if "dest" in op and op["dest"] is not None:
-        with salt.utils.files.fopen(op["dest"], "w") as fp:
-            fp.write(salt.utils.stringutils.to_str(result))
+        try:
+            with salt.utils.files.fopen(op["dest"], "w") as fp:
+                fp.write(salt.utils.stringutils.to_str(result))
+        except IOError:
+            ret["message"] = 'Unable to open "{0}" to write'.format(op["dest"])
+            ret["out"] = False
+            return ret
 
     ret["out"] = True
     return ret
@@ -803,7 +825,9 @@ def install_config(path=None, **kwargs):
     diffs_file
       Path to the file where the diff (difference in old configuration and the
       committed configuration) will be stored. Note that the file will be
-      stored on the proxy minion. To push the files to the master use
+      stored on the proxy minion. To push the files to the master
+
+      use
       :py:func:`cp.push <salt.modules.cp.push>`.
 
     template_vars
@@ -848,8 +872,18 @@ def install_config(path=None, **kwargs):
     if "template_vars" in op:
         template_vars = op["template_vars"]
 
-    template_cached_path = salt.utils.files.mkstemp()
-    __salt__["cp.get_template"](path, template_cached_path, template_vars=template_vars)
+    try:
+        template_cached_path = salt.utils.files.mkstemp()
+        __salt__["cp.get_template"](
+            path, template_cached_path, template_vars=template_vars
+        )
+    except Exception as ex:  # pylint: disable=broad-except
+        ret["message"] = (
+            "Salt failed to render the template, please check file path and syntax."
+            "\nError: {0}".format(str(ex))
+        )
+        ret["out"] = False
+        return ret
 
     if not os.path.isfile(template_cached_path):
         ret["message"] = "Invalid file path."
@@ -888,77 +922,94 @@ def install_config(path=None, **kwargs):
         del op["overwrite"]
 
     db_mode = op.pop("mode", "exclusive")
-    with Config(conn, mode=db_mode) as cu:
-        try:
-            cu.load(**op)
-
-        except Exception as exception:  # pylint: disable=broad-except
-            ret["message"] = 'Could not load configuration due to : "{0}"'.format(
-                exception
-            )
-            ret["format"] = op["format"]
-            ret["out"] = False
-            return ret
-
-        finally:
-            salt.utils.files.safe_rm(template_cached_path)
-
-        config_diff = cu.diff()
-        if config_diff is None:
-            ret["message"] = "Configuration already applied!"
-            ret["out"] = True
-            return ret
-
-        commit_params = {}
-        if "confirm" in op:
-            commit_params["confirm"] = op["confirm"]
-        if "comment" in op:
-            commit_params["comment"] = op["comment"]
-
-        try:
-            check = cu.commit_check()
-        except Exception as exception:  # pylint: disable=broad-except
-            ret["message"] = 'Commit check threw the following exception: "{0}"'.format(
-                exception
-            )
-
-            ret["out"] = False
-            return ret
+    if write_diff and db_mode == "dynamic":
+        ret["message"] = "Write diff is not supported with dynamic configuration mode"
+        ret["out"] = False
+        return ret
 
-        if check and not test:
+    try:
+        with Config(conn, mode=db_mode) as cu:
             try:
-                cu.commit(**commit_params)
-                ret["message"] = "Successfully loaded and committed!"
+                cu.load(**op)
             except Exception as exception:  # pylint: disable=broad-except
+                ret["message"] = 'Could not load configuration due to : "{0}"'.format(
+                    exception
+                )
+                ret["format"] = op["format"]
+                ret["out"] = False
+                return ret
+            finally:
+                salt.utils.files.safe_rm(template_cached_path)
+
+            if db_mode != "dynamic":
+                config_diff = cu.diff()
+                if config_diff is None:
+                    ret["message"] = "Configuration already applied!"
+                    ret["out"] = True
+                    return ret
+
+            commit_params = {}
+            if "confirm" in op:
+                commit_params["confirm"] = op["confirm"]
+            if "comment" in op:
+                commit_params["comment"] = op["comment"]
+
+            # Assume commit_check succeeds and initialize variable check
+            check = True
+            if db_mode != "dynamic":
+                try:
+                    check = cu.commit_check()
+                except Exception as exception:  # pylint: disable=broad-except
+                    ret[
+                        "message"
+                    ] = 'Commit check threw the following exception: "{0}"'.format(
+                        exception
+                    )
+                    ret["out"] = False
+                    return ret
+
+            if check and not test:
+                try:
+                    cu.commit(**commit_params)
+                    ret["message"] = "Successfully loaded and committed!"
+                except Exception as exception:  # pylint: disable=broad-except
+                    ret[
+                        "message"
+                    ] = 'Commit check successful but commit failed with "{0}"'.format(
+                        exception
+                    )
+                    ret["out"] = False
+                    return ret
+            elif not check:
+                cu.rollback()
                 ret[
                     "message"
-                ] = 'Commit check successful but commit failed with "{0}"'.format(
+                ] = "Loaded configuration but commit check failed, hence rolling back configuration."
+                ret["out"] = False
+            else:
+                cu.rollback()
+                ret[
+                    "message"
+                ] = "Commit check passed, but skipping commit for dry-run and rolling back configuration."
+                ret["out"] = True
+            try:
+                if write_diff and config_diff is not None:
+                    with salt.utils.files.fopen(write_diff, "w") as fp:
+                        fp.write(salt.utils.stringutils.to_str(config_diff))
+            except Exception as exception:  # pylint: disable=broad-except
+                ret["message"] = 'Could not write into diffs_file due to: "{0}"'.format(
                     exception
                 )
                 ret["out"] = False
-                return ret
-        elif not check:
-            cu.rollback()
-            ret[
-                "message"
-            ] = "Loaded configuration but commit check failed, hence rolling back configuration."
-            ret["out"] = False
-        else:
-            cu.rollback()
-            ret[
-                "message"
-            ] = "Commit check passed, but skipping commit for dry-run and rolling back configuration."
-            ret["out"] = True
-
-        try:
-            if write_diff and config_diff is not None:
-                with salt.utils.files.fopen(write_diff, "w") as fp:
-                    fp.write(salt.utils.stringutils.to_str(config_diff))
-        except Exception as exception:  # pylint: disable=broad-except
-            ret["message"] = 'Could not write into diffs_file due to: "{0}"'.format(
-                exception
-            )
-            ret["out"] = False
+    except ValueError:
+        ret[
+            "message"
+        ] = "Invalid mode. Modes supported: private, dynamic, batch, exclusive"
+        ret["out"] = False
+    except LockError as ex:
+        log.error("Configuration database is locked")
+        ret["message"] = ex.message
+        ret["out"] = False
 
     return ret
 
@@ -967,6 +1018,13 @@ def zeroize():
     """
     Resets the device to default factory settings
 
+    .. note::
+        In case of non-root user, proxy_reconnect will not be able
+        to re-connect to the device as zeroize will delete the local
+        user's configuration.
+        For more details on zeroize functionality, please refer
+        https://www.juniper.net/documentation/en_US/junos/topics/reference/command-summary/request-system-zeroize.html
+
     CLI Example:
 
     .. code-block:: bash
@@ -989,14 +1047,13 @@ def zeroize():
 @timeoutDecorator
 def install_os(path=None, **kwargs):
     """
-    Installs the given image on the device. After the installation is complete\
-     the device is rebooted,
-    if reboot=True is given as a keyworded argument.
+    Installs the given image on the device. After the installation is complete
+    the device is rebooted, if reboot=True is given as a keyworded argument.
 
     path (required)
         Path where the image file is present on the proxy minion
 
-    remote_path :
+    remote_path : /var/tmp
         If the value of path  is a file path on the local
         (Salt host's) filesystem, then the image is copied from the local
         filesystem to the :remote_path: directory on the target Junos
@@ -1020,19 +1077,28 @@ def install_os(path=None, **kwargs):
         When ``True`` this method will perform a config validation against
         the new image
 
-    bool issu:
+    bool issu: False
         When ``True`` allows unified in-service software upgrade
         (ISSU) feature enables you to upgrade between two different Junos OS
         releases with no disruption on the control plane and with minimal
         disruption of traffic.
 
-    bool nssu:
+    bool nssu: False
         When ``True`` allows nonstop software upgrade (NSSU)
         enables you to upgrade the software running on a Juniper Networks
         EX Series Virtual Chassis or a Juniper Networks EX Series Ethernet
         Switch with redundant Routing Engines with a single command and
         minimal disruption to network traffic.
 
+    bool all_re: True
+        When True (default), executes the software install on all Routing Engines of the Junos
+        device. When False, execute the software install only on the current Routing Engine.
+
+    .. note::
+        Any additional keyword arguments specified are passed down to PyEZ sw.install() as is.
+        Please refer to below URl for PyEZ sw.install() documentaion:
+        https://pyez.readthedocs.io/en/latest/jnpr.junos.utils.html#jnpr.junos.utils.sw.SW.install
+
     CLI Examples:
 
     .. code-block:: bash
@@ -1052,6 +1118,14 @@ def install_os(path=None, **kwargs):
     else:
         op.update(kwargs)
 
+    # timeout value is not honoured by sw.install if not passed as argument
+    # currently, timeout is set to be maximum of default 1800 and user passed timeout value
+    # For info: https://github.com/Juniper/salt/issues/116
+    op.pop("dev_timeout", None)
+    timeout = max(1800, conn.timeout)
+    # Reboot should not be passed as a keyword argument to install(),
+    # Please refer to https://github.com/Juniper/salt/issues/115 for more details
+    reboot = op.pop("reboot", False)
     no_copy_ = op.get("no_copy", False)
 
     if path is None:
@@ -1062,22 +1136,29 @@ def install_os(path=None, **kwargs):
         return ret
 
     if not no_copy_:
-        image_cached_path = salt.utils.files.mkstemp()
-        __salt__["cp.get_file"](path, image_cached_path)
+        # To handle invalid image path scenario
+        try:
+            image_cached_path = salt.utils.files.mkstemp()
+            __salt__["cp.get_file"](path, image_cached_path)
 
-        if not os.path.isfile(image_cached_path):
-            ret["message"] = "Invalid image path."
-            ret["out"] = False
-            return ret
+            if not os.path.isfile(image_cached_path):
+                ret["message"] = "Invalid image path."
+                ret["out"] = False
+                return ret
 
-        if os.path.getsize(image_cached_path) == 0:
-            ret["message"] = "Failed to copy image"
+            if os.path.getsize(image_cached_path) == 0:
+                ret["message"] = "Failed to copy image"
+                ret["out"] = False
+                return ret
+            path = image_cached_path
+        except MinionError:
+            ret["message"] = "Invalid path. Please provide a valid image path"
             ret["out"] = False
             return ret
-        path = image_cached_path
 
+    # install() should not reboot the device, reboot is handled in the next block
     try:
-        conn.sw.install(path, progress=True, **op)
+        conn.sw.install(path, progress=True, timeout=timeout, **op)
         ret["message"] = "Installed the os."
     except Exception as exception:  # pylint: disable=broad-except
         ret["message"] = 'Installation failed due to: "{0}"'.format(exception)
@@ -1087,7 +1168,8 @@ def install_os(path=None, **kwargs):
         if not no_copy_:
             salt.utils.files.safe_rm(image_cached_path)
 
-    if "reboot" in op and op["reboot"] is True:
+    # Handle reboot, after the install has finished
+    if reboot is True:
         try:
             conn.sw.reboot()
         except Exception as exception:  # pylint: disable=broad-except
@@ -1223,6 +1305,9 @@ def load(path=None, **kwargs):
         ``True``, only those statements under the ``replace`` tag will be
         changed.
 
+    merge : False
+        If set to ``True`` will set the load-config action to merge.
+
     format
         Determines the format of the contents
 
@@ -1275,12 +1360,21 @@ def load(path=None, **kwargs):
     else:
         op.update(kwargs)
 
-    template_vars = {}
+    kwargs = {}
     if "template_vars" in op:
-        template_vars = op["template_vars"]
+        kwargs = op["template_vars"]
 
-    template_cached_path = salt.utils.files.mkstemp()
-    __salt__["cp.get_template"](path, template_cached_path, template_vars=template_vars)
+    try:
+        template_cached_path = salt.utils.files.mkstemp()
+        __salt__["cp.get_template"](
+            path,
+            template_cached_path,
+            **kwargs)
+    except Exception as ex:  # pylint: disable=broad-except
+        ret["message"] = "Salt failed to render the template, please check file path and syntax." \
+                         "\nError: {0}".format(str(ex))
+        ret["out"] = False
+        return ret
 
     if not os.path.isfile(template_cached_path):
         ret["message"] = "Invalid file path."
@@ -1304,11 +1398,27 @@ def load(path=None, **kwargs):
 
         op["format"] = template_format
 
+    # Currently, four config_actions are supported: overwrite, replace, update, merge
+    # Allow only one config_action, providing multiple config_action value is not allowed
+    actions = [
+        item
+        for item in ('overwrite', 'replace', 'update', 'merge')
+        if op.get(item, False)
+    ]
+    if len(list(actions)) > 1:
+        ret["message"] = "Only one config_action is allowed. Provided: {0}".format(
+            actions
+        )
+        ret["out"] = False
+        return ret
+
     if "replace" in op and op["replace"]:
         op["merge"] = False
         del op["replace"]
     elif "overwrite" in op and op["overwrite"]:
         op["overwrite"] = True
+    elif "merge" in op and op["merge"]:
+        op["merge"] = True
     elif "overwrite" in op and not op["overwrite"]:
         op["merge"] = True
         del op["overwrite"]
@@ -1348,3 +1458,131 @@ def commit_check():
         ret["out"] = False
 
     return ret
+
+
+def get_table(
+    table,
+    table_file,
+    path=None,
+    target=None,
+    key=None,
+    key_items=None,
+    filters=None,
+    template_args=None
+):
+    """
+    Retrieve data from a Junos device using Tables/Views
+
+    table (required)
+        Name of PyEZ Table
+
+    table_file (required)
+        YAML file that has the table specified in table parameter
+
+    path:
+        Path of location of the YAML file.
+        defaults to op directory in jnpr.junos.op
+
+    target:
+        if command need to run on FPC, can specify fpc target
+
+    key:
+        To overwrite key provided in YAML
+
+    key_items:
+        To select only given key items
+
+    filters:
+        To select only filter for the dictionary from columns
+
+    template_args:
+        key/value pair which should render Jinja template command
+
+    CLI Example:
+
+    .. code-block:: bash
+
+        salt 'device_name' junos.get_table
+    """
+    conn = __proxy__["junos.conn"]()
+    ret = {}
+    ret["out"] = True
+    ret["hostname"] = conn._hostname
+    ret["tablename"] = table
+    get_kvargs = {}
+    if target is not None:
+        get_kvargs["target"] = target
+    if key is not None:
+        get_kvargs["key"] = key
+    if key_items is not None:
+        get_kvargs["key_items"] = key_items
+    if filters is not None:
+        get_kvargs["filters"] = filters
+    if template_args is not None and isinstance(template_args, dict):
+        get_kvargs["args"] = template_args
+    pyez_tables_path = os.path.dirname(os.path.abspath(tables_dir.__file__))
+    try:
+        if path is not None:
+            file_loc = glob.glob(os.path.join(path, "{}".format(table_file)))
+        else:
+            file_loc = glob.glob(
+                os.path.join(pyez_tables_path, "{}".format(table_file))
+            )
+        if len(file_loc) == 1:
+            file_name = file_loc[0]
+        else:
+            ret["message"] = "Given table file {} cannot be located".format(table_file)
+            ret["out"] = False
+            return ret
+        try:
+            with salt.utils.files.fopen(file_name) as fp:
+                ret["table"] = yaml.load(fp.read(), Loader=yamlordereddictloader.Loader)
+                globals().update(FactoryLoader().load(ret["table"]))
+        except IOError as err:
+            ret["message"] = (
+                "Uncaught exception during YAML Load - please report: {0}".format(
+                    six.text_type(err))
+            )
+            ret["out"] = False
+            return ret
+        try:
+            data = globals()[table](conn)
+            data.get(**get_kvargs)
+        except KeyError as err:
+            ret["message"] = (
+                "Uncaught exception during get API call - please report: {0}".format
+                (six.text_type(err))
+            )
+            ret["out"] = False
+            return ret
+        except ConnectClosedError:
+            ret["message"] = (
+                "Got ConnectClosedError exception. Connection lost with {}".format(
+                    conn)
+            )
+            ret["out"] = False
+            return ret
+        ret["reply"] = json.loads(data.to_json())
+        if data.__class__.__bases__[0] == OpTable:
+            # Sets key value if not present in YAML. To be used by returner
+            if ret["table"][table].get("key") is None:
+                ret["table"][table]["key"] = data.ITEM_NAME_XPATH
+            # If key is provided from salt state file.
+            if key is not None:
+                ret["table"][table]["key"] = data.KEY
+        else:
+            if target is not None:
+                ret["table"][table]["target"] = data.TARGET
+            if key is not None:
+                ret["table"][table]["key"] = data.KEY
+            if key_items is not None:
+                ret["table"][table]["key_items"] = data.KEY_ITEMS
+            if template_args is not None:
+                ret["table"][table]["args"] = data.CMD_ARGS
+                ret["table"][table]["command"] = data.GET_CMD
+    except Exception as err:  # pylint: disable=broad-except
+        ret["message"] = "Uncaught exception - please report: {0}".format(str(err))
+        traceback.print_exc()
+        ret["out"] = False
+        return ret
+    return ret
diff --git a/salt/proxy/junos.py b/salt/proxy/junos.py
index a68f03bfaea9..d12328f553b9 100644
--- a/salt/proxy/junos.py
+++ b/salt/proxy/junos.py
@@ -114,7 +114,7 @@ def init(opts):
     for arg in optional_args:
         if arg in proxy_keys:
             args[arg] = opts["proxy"][arg]
-
+    log.debug("Args: {}".format(args))
     thisproxy["conn"] = jnpr.junos.Device(**args)
     try:
         thisproxy["conn"].open()
diff --git a/salt/states/junos.py b/salt/states/junos.py
index e9cb4928e594..5cb54c262c67 100644
--- a/salt/states/junos.py
+++ b/salt/states/junos.py
@@ -33,7 +33,7 @@ def wrapper(*args, **kwargs):
 
 
 @resultdecorator
-def rpc(name, dest=None, format="xml", args=None, **kwargs):
+def rpc(name, **kwargs):
     """
     Executes the given rpc. The returned data can be stored in a file
     by specifying the destination path with dest as an argument
@@ -41,8 +41,7 @@ def rpc(name, dest=None, format="xml", args=None, **kwargs):
     .. code-block:: yaml
 
         get-interface-information:
-            junos:
-              - rpc
+            junos.rpc:
               - dest: /home/user/rpc.log
               - interface_name: lo0
 
@@ -61,7 +60,7 @@ def rpc(name, dest=None, format="xml", args=None, **kwargs):
           The format in which the rpc reply must be stored in file specified in the dest
           (used only when dest is specified) (default = xml)
         * kwargs: keyworded arguments taken by rpc call like-
-            * timeout:
+            * timeout: 30
               Set NETCONF RPC timeout. Can be used for commands which
               take a while to execute. (default= 30 seconds)
             * filter:
@@ -72,10 +71,7 @@ def rpc(name, dest=None, format="xml", args=None, **kwargs):
               Name of the interface whose information you want.
     """
     ret = {"name": name, "changes": {}, "result": True, "comment": ""}
-    if args is not None:
-        ret["changes"] = __salt__["junos.rpc"](name, dest, format, *args, **kwargs)
-    else:
-        ret["changes"] = __salt__["junos.rpc"](name, dest, format, **kwargs)
+    ret['changes'] = __salt__["junos.rpc"](name, **kwargs)
     return ret
 
 
@@ -87,8 +83,7 @@ def set_hostname(name, **kwargs):
     .. code-block:: yaml
 
             device_name:
-              junos:
-                - set_hostname
+              junos.set_hostname:
                 - comment: "Host-name set via saltstack."
 
 
@@ -121,8 +116,7 @@ def commit(name, **kwargs):
     .. code-block:: yaml
 
             commit the changes:
-              junos:
-                - commit
+              junos.commit:
                 - confirm: 10
 
 
@@ -158,15 +152,14 @@ def commit(name, **kwargs):
 
 
 @resultdecorator
-def rollback(name, id, **kwargs):
+def rollback(name, **kwargs):
     """
     Rollbacks the committed changes.
 
     .. code-block:: yaml
 
             rollback the changes:
-              junos:
-                - rollback
+              junos.rollback:
                 - id: 5
 
     Parameters:
@@ -188,20 +181,19 @@ def rollback(name, id, **kwargs):
 
     """
     ret = {"name": name, "changes": {}, "result": True, "comment": ""}
-    ret["changes"] = __salt__["junos.rollback"](id, **kwargs)
+    ret["changes"] = __salt__["junos.rollback"](**kwargs)
     return ret
 
 
 @resultdecorator
-def diff(name, d_id):
+def diff(name, **kwargs):
     """
     Gets the difference between the candidate and the current configuration.
 
     .. code-block:: yaml
 
             get the diff:
-              junos:
-                - diff
+              junos.diff:
                 - id: 10
 
     Parameters:
@@ -210,7 +202,7 @@ def diff(name, d_id):
           The rollback id value [0-49]. (default = 0)
     """
     ret = {"name": name, "changes": {}, "result": True, "comment": ""}
-    ret["changes"] = __salt__["junos.diff"](d_id)
+    ret["changes"] = __salt__["junos.diff"](**kwargs)
     return ret
 
 
@@ -222,8 +214,7 @@ def cli(name, **kwargs):
     .. code-block:: yaml
 
             show version:
-              junos:
-                - cli
+              junos.cli:
                 - format: xml
 
     Parameters:
@@ -255,8 +246,7 @@ def shutdown(name, **kwargs):
     .. code-block:: yaml
 
             shut the device:
-              junos:
-                - shutdown
+              junos.shutdown:
                 - in_min: 10
 
     Parameters:
@@ -282,8 +272,7 @@ def install_config(name, **kwargs):
     .. code-block:: yaml
 
             Install the mentioned config:
-              junos:
-                - install_config
+              junos.install_config:
                 - path: salt//configs/interface.set
                 - timeout: 100
                 - diffs_file: 'var/log/diff'
@@ -292,8 +281,7 @@ def install_config(name, **kwargs):
     .. code-block:: yaml
 
             Install the mentioned config:
-              junos:
-                - install_config
+              junos.install_config:
                 - template_path: salt//configs/interface.set
                 - timeout: 100
                 - template_vars:
@@ -372,8 +360,7 @@ def install_os(name, **kwargs):
     .. code-block:: yaml
 
             salt://images/junos_image.tgz:
-              junos:
-                - install_os
+              junos.install_os:
                 - timeout: 100
                 - reboot: True
 
@@ -407,8 +394,7 @@ def file_copy(name, dest=None, **kwargs):
     .. code-block:: yaml
 
             /home/m2/info.txt:
-              junos:
-                - file_copy
+              junos.file_copy:
                 - dest: info_copy.txt
 
     Parameters:
@@ -468,20 +454,24 @@ def load(name, **kwargs):
 
     .. code-block:: yaml
 
-            Install the mentioned config:
-              junos:
-                - load
-                - path: salt//configs/interface.set
+        Install the mentioned config:
+          junos.load:
+            - path: salt//configs/interface.set
 
     .. code-block:: yaml
 
-            Install the mentioned config:
-              junos:
-                - load
-                - template_path: salt//configs/interface.set
-                - template_vars:
-                    interface_name: lo0
-                    description: Creating interface via SaltStack.
+        Install the mentioned config:
+          junos.load:
+            - template_path: salt//configs/interface.set
+            - template_vars:
+                interface_name: lo0
+                description: Creating interface via SaltStack.
+
+    Sample template:
+
+    .. code-block:: bash
+
+        set interfaces {{ interface_name }} unit 0
 
 
     name
@@ -538,3 +528,49 @@ def commit_check(name):
     ret = {"name": name, "changes": {}, "result": True, "comment": ""}
     ret["changes"] = __salt__["junos.commit_check"]()
     return ret
+
+
+@resultdecorator
+def get_table(name, table, table_file, **kwargs):
+    """
+    Retrieve data from a Junos device using Tables/Views
+
+    .. code-block:: yaml
+
+        get route details:
+          junos.get_table:
+            - table: RouteTable
+            - file: routes.yml
+
+    name (required)
+        task definition
+
+    table (required)
+        Name of PyEZ Table
+
+    file
+        YAML file that has the table specified in table parameter
+
+    path:
+        Path of location of the YAML file.
+        defaults to op directory in jnpr.junos.op
+
+    target:
+        if command need to run on FPC, can specify fpc target
+
+    key:
+        To overwrite key provided in YAML
+
+    key_items:
+        To select only given key items
+
+    filters:
+        To select only filter for the dictionary from columns
+
+    template_args:
+        key/value pair which should render Jinja template command
+
+    """
+    ret = {"name": name, "changes": {}, "result": True, "comment": ""}
+    ret["changes"] = __salt__["junos.get_table"](table, table_file, **kwargs)
+    return ret
diff --git a/tests/unit/modules/test_junos.py b/tests/unit/modules/test_junos.py
index 0e72802e4687..73c1f35c2750 100644
--- a/tests/unit/modules/test_junos.py
+++ b/tests/unit/modules/test_junos.py
@@ -4,9 +4,11 @@
 """
 # Import python libs
 from __future__ import absolute_import, print_function, unicode_literals
+import os
 
 # Import salt modules
 import salt.modules.junos as junos
+from salt.ext import six
 
 # Import test libs
 from tests.support.mixins import LoaderModuleMockMixin, XMLEqualityMixin
@@ -24,8 +26,8 @@
     from jnpr.junos.utils.sw import SW
     from jnpr.junos.device import Device
     import jxmlease  # pylint: disable=unused-import
-    from jnpr.junos.exception import LockError, UnlockError
-
+    import jnpr.junos.op as tables_dir
+    from jnpr.junos.exception import ConnectClosedError, LockError, UnlockError
     HAS_JUNOS = True
 except ImportError:
     HAS_JUNOS = False
@@ -954,6 +956,51 @@ def test_cli_exception_in_cli(self):
             ret["out"] = False
             self.assertEqual(junos.cli("show version"), ret)
 
+    def test_cli_output_save(self):
+        with patch("jnpr.junos.device.Device.cli") as mock_cli, patch(
+            "salt.utils.files.fopen"
+        ) as mock_fopen:
+            mock_cli.return_value = "Test return"
+            args = {
+                "__pub_user": "root",
+                "__pub_arg": [{"format": "text", "dest": "/path/to/file"}],
+                "format": "text",
+                "dest": "/path/to/file",
+                "__pub_fun": "junos.cli",
+                "__pub_jid": "20170221182531323467",
+                "__pub_tgt": "mac_min",
+                "__pub_tgt_type": "glob",
+                "__pub_ret": ""
+            }
+            ret = dict()
+            ret["message"] = "Test return"
+            ret["out"] = True
+            self.assertEqual(junos.cli("show version", **args), ret)
+            mock_fopen.assert_called_with("/path/to/file", "w")
+            mock_cli.assert_called_with("show version", "text", warning=False)
+
+    def test_cli_output_save_ioexception(self):
+        with patch("jnpr.junos.device.Device.cli") as mock_cli, patch(
+            "salt.utils.files.fopen"
+        ) as mock_fopen:
+            mock_cli.return_value = "Test return"
+            mock_fopen.side_effect = IOError()
+            args = {
+                "__pub_user": "root",
+                "__pub_arg": [{"format": "text", "dest": "/path/to/file"}],
+                "format": "text",
+                "dest": "/path/to/file",
+                "__pub_fun": "junos.cli",
+                "__pub_jid": "20170221182531323467",
+                "__pub_tgt": "mac_min",
+                "__pub_tgt_type": "glob",
+                "__pub_ret": ""
+            }
+            ret = dict()
+            ret["message"] = 'Unable to open "/path/to/file" to write'
+            ret["out"] = False
+            self.assertEqual(junos.cli("show version", **args), ret)
+
     def test_shutdown_without_args(self):
         ret = dict()
         ret["message"] = "Provide either one of the arguments: shutdown or reboot."
@@ -1196,14 +1243,10 @@ def test_install_config_replace(self):
             mock_commit_check.return_value = True
 
             args = {
-                "__pub_user": "root",
-                "__pub_arg": [{"replace": True}],
-                "replace": True,
-                "__pub_fun": "junos.install_config",
-                "__pub_jid": "20170222213858582619",
-                "__pub_tgt": "mac_min",
-                "__pub_tgt_type": "glob",
-                "__pub_ret": "",
+                "__pub_user": "root", "__pub_arg": [{"replace": True}],
+                "replace": True, "__pub_fun": "junos.install_config",
+                "__pub_jid": "20170222213858582619", "__pub_tgt": "mac_min",
+                "__pub_tgt_type": "glob", "__pub_ret": ""
             }
 
             ret = dict()
@@ -1518,6 +1561,100 @@ def test_install_config_commit_exception(self):
             ret["out"] = False
             self.assertEqual(junos.install_config("actual/path/config"), ret)
 
+    def test_install_config_test_mode(self):
+        with patch("jnpr.junos.utils.config.Config.commit") as mock_commit, patch(
+            "jnpr.junos.utils.config.Config.commit_check"
+        ) as mock_commit_check, patch(
+            "jnpr.junos.utils.config.Config.diff"
+        ) as mock_diff, patch(
+            "jnpr.junos.utils.config.Config.load"
+        ) as mock_load, patch(
+            "salt.utils.files.safe_rm"
+        ) as mock_safe_rm, patch(
+            "salt.utils.files.mkstemp"
+        ) as mock_mkstemp, patch(
+            "os.path.isfile"
+        ) as mock_isfile, patch(
+            "os.path.getsize"
+        ) as mock_getsize:
+            mock_isfile.return_value = True
+            mock_getsize.return_value = 10
+            mock_mkstemp.return_value = "test/path/config"
+            mock_diff.return_value = "diff"
+            mock_commit_check.return_value = True
+            ret = dict()
+            ret[
+                "message"
+            ] = "Commit check passed, but skipping commit for dry-run and rolling back configuration."
+            ret["out"] = True
+            self.assertEqual(junos.install_config("actual/path/config", test=True), ret)
+            mock_commit.assert_not_called()
+
+    def test_install_config_write_diff_dynamic_mode(self):
+        with patch("jnpr.junos.utils.config.Config.commit") as mock_commit, patch(
+            "jnpr.junos.utils.config.Config.commit_check"
+        ) as mock_commit_check, patch(
+            "jnpr.junos.utils.config.Config.diff"
+        ) as mock_diff, patch(
+            "jnpr.junos.utils.config.Config.load"
+        ) as mock_load, patch(
+            "salt.utils.files.safe_rm"
+        ) as mock_safe_rm, patch(
+            "salt.utils.files.mkstemp"
+        ) as mock_mkstemp, patch(
+            "os.path.isfile"
+        ) as mock_isfile, patch(
+            "os.path.getsize"
+        ) as mock_getsize:
+            mock_isfile.return_value = True
+            mock_getsize.return_value = 10
+            mock_mkstemp.return_value = "test/path/config"
+            mock_diff.return_value = "diff"
+            mock_commit_check.return_value = True
+            ret = dict()
+            ret[
+                "message"
+            ] = "Write diff is not supported with dynamic configuration mode"
+            ret["out"] = False
+            self.assertEqual(
+                junos.install_config(
+                    "actual/path/config", mode="dynamic", diffs_file="/path/to/dif"
+                ),
+                ret,
+            )
+            mock_commit.assert_not_called()
+
+    def test_install_config_unknown_mode(self):
+        with patch("jnpr.junos.utils.config.Config.commit") as mock_commit, patch(
+            "jnpr.junos.utils.config.Config.commit_check"
+        ) as mock_commit_check, patch(
+            "jnpr.junos.utils.config.Config.diff"
+        ) as mock_diff, patch(
+            "jnpr.junos.utils.config.Config.load"
+        ) as mock_load, patch(
+            "salt.utils.files.safe_rm"
+        ) as mock_safe_rm, patch(
+            "salt.utils.files.mkstemp"
+        ) as mock_mkstemp, patch(
+            "os.path.isfile"
+        ) as mock_isfile, patch(
+            "os.path.getsize"
+        ) as mock_getsize:
+            mock_isfile.return_value = True
+            mock_getsize.return_value = 10
+            mock_mkstemp.return_value = "test/path/config"
+            mock_diff.return_value = "diff"
+            mock_commit_check.return_value = True
+            ret = dict()
+            ret[
+                "message"
+            ] = "Invalid mode. Modes supported: private, dynamic, batch, exclusive"
+            ret["out"] = False
+            self.assertEqual(
+                junos.install_config("actual/path/config", mode="abcdef"), ret
+            )
+            mock_commit.assert_not_called()
+
     def test_zeroize(self):
         with patch("jnpr.junos.device.Device.cli") as mock_cli:
             result = junos.zeroize()
@@ -1667,7 +1804,9 @@ def test_install_os_no_copy(self):
             ret["out"] = True
             ret["message"] = "Installed the os."
             self.assertEqual(junos.install_os("path", no_copy=True), ret)
-            mock_install.assert_called_with("path", no_copy=True, progress=True)
+            mock_install.assert_called_with(
+                "path", no_copy=True, progress=True, timeout=1800
+            )
             mock_mkstemp.assert_not_called()
             mock_safe_rm.assert_not_called()
 
@@ -1685,7 +1824,7 @@ def test_install_os_issu(self):
             ret["out"] = True
             ret["message"] = "Installed the os."
             self.assertEqual(junos.install_os("path", issu=True), ret)
-            mock_install.assert_called_with(ANY, issu=True, progress=True)
+            mock_install.assert_called_with(ANY, issu=True, progress=True, timeout=1800)
 
     def test_install_os_add_params(self):
         with patch("jnpr.junos.utils.sw.SW.install") as mock_install, patch(
@@ -1708,7 +1847,12 @@ def test_install_os_add_params(self):
                 ret,
             )
             mock_install.assert_called_with(
-                ANY, nssu=True, remote_path=remote_path, progress=True, validate=True
+                ANY,
+                nssu=True,
+                remote_path=remote_path,
+                progress=True,
+                validate=True,
+                timeout=1800
             )
 
     def test_file_copy_without_args(self):
@@ -1764,8 +1908,9 @@ def test_virtual_proxy_unavailable(self):
         with patch.dict(junos.__opts__, {}):
             res = (
                 False,
-                "The junos module could not be "
-                "loaded: junos-eznc or jxmlease or proxy could not be loaded.",
+                "The junos or dependent module could not be loaded: "
+                "junos-eznc or jxmlease or or yamlordereddictloader or "
+                "proxy could not be loaded."
             )
             self.assertEqual(junos.__virtual__(), res)
 
@@ -1806,7 +1951,7 @@ def test_rpc_get_config_filter(self):
             junos.rpc("get-config", **args)
             exec_args = mock_execute.call_args
             expected_rpc = (
-                '<get-configuration dev_timeout="30" '
+                "<get-configuration "
                 'format="xml"><configuration><system/></configuration></get-configuration>'
             )
             self.assertEqualXML(exec_args[0][0], expected_rpc)
@@ -1826,8 +1971,9 @@ def test_rpc_get_interface_information_with_kwargs(self):
                     "get-interface-information",
                     "",
                     "text",
-                    {"terse": True, "interface_name": "lo0"},
+                    {"terse": True, "interface_name": "lo0", "format": "text"},
                 ],
+                "format": "text",
                 "terse": True,
                 "__pub_fun": "junos.rpc",
                 "__pub_jid": "20170314160943363563",
@@ -1836,7 +1982,7 @@ def test_rpc_get_interface_information_with_kwargs(self):
                 "__pub_tgt_type": "glob",
                 "__pub_ret": "",
             }
-            junos.rpc("get-interface-information", format="text", **args)
+            junos.rpc("get-interface-information", **args)
             args = mock_execute.call_args
             expected_rpc = (
                 '<get-interface-information format="text">'
@@ -1874,7 +2020,7 @@ def test_rpc_write_file_format_text(self):
                 "<rpc-reply>text rpc reply</rpc-reply>"
             )
             with patch("salt.utils.files.fopen", mock_open(), create=True) as m_open:
-                junos.rpc("get-chassis-inventory", "/path/to/file", format="text")
+                junos.rpc("get-chassis-inventory", dest="/path/to/file", format="text")
                 writes = m_open.write_calls()
                 assert writes == ["text rpc reply"], writes
 
@@ -1884,7 +2030,7 @@ def test_rpc_write_file_format_json(self):
         ) as mock_dumps:
             mock_dumps.return_value = "json rpc reply"
             with patch("salt.utils.files.fopen", mock_open(), create=True) as m_open:
-                junos.rpc("get-chassis-inventory", "/path/to/file", format="json")
+                junos.rpc("get-chassis-inventory", dest="/path/to/file", format="json")
                 writes = m_open.write_calls()
                 assert writes == ["json rpc reply"], writes
 
@@ -1894,7 +2040,7 @@ def test_rpc_write_file(self):
         ) as mock_tostring, patch("jnpr.junos.device.Device.execute") as mock_execute:
             mock_tostring.return_value = "xml rpc reply"
             with patch("salt.utils.files.fopen", mock_open(), create=True) as m_open:
-                junos.rpc("get-chassis-inventory", "/path/to/file")
+                junos.rpc("get-chassis-inventory", dest="/path/to/file")
                 writes = m_open.write_calls()
                 assert writes == ["xml rpc reply"], writes
 
@@ -2080,3 +2226,110 @@ def test_commit_check_error(self):
             mock_check.side_effect = Exception
             ret = junos.commit_check()
             self.assertEqual(ret, ret_exp)
+
+    def test_get_table_wrong_path(self):
+        table = "ModuleTable"
+        file = "sample.yml"
+        path = "/path/to/file"
+        ret_exp = {
+            "out": False,
+            "hostname": "1.1.1.1",
+            "tablename": "ModuleTable",
+            "message": "Given table file {} cannot be located".format(file),
+        }
+        with patch("jnpr.junos.factory.FactoryLoader.load") as mock_load:
+            ret = junos.get_table(table, file, path)
+            self.assertEqual(ret, ret_exp)
+            mock_load.assert_not_called()
+
+    def test_get_table_no_path_no_file(self):
+        table = "ModuleTable"
+        file = "inventory.yml"
+        ret_exp = {
+            "out": False,
+            "hostname": "1.1.1.1",
+            "tablename": "ModuleTable",
+            "message": "Given table file {} cannot be located".format(file),
+        }
+        with patch("jnpr.junos.factory.FactoryLoader.load") as mock_load, patch(
+            "glob.glob"
+        ) as mock_fopen:
+            mock_fopen.return_value = []
+            ret = junos.get_table(table, file)
+            self.assertEqual(ret, ret_exp)
+            mock_load.assert_not_called()
+
+    def test_get_table_yaml_load_error(self):
+        table = "ModuleTable"
+        file = "inventory.yml"
+        path = "/path/to/file"
+        message = "File not located test"
+        ret_exp = {
+            "out": False,
+            "hostname": "1.1.1.1",
+            "tablename": "ModuleTable",
+           "message": "Uncaught exception during YAML Load - please report: {}".format(
+               message),
+        }
+        with patch(
+            "salt.utils.files.fopen", mock_open(IOError(message))
+        ) as mock_file, patch("glob.glob") as mock_fopen:
+            mock_fopen.return_value = ["/path/to/file"]
+            ret = junos.get_table(table, file, path)
+            self.assertEqual(ret, ret_exp)
+
+    def test_get_table_api_error(self):
+        table = str("sample")
+        file = "inventory.yml"
+        ret_exp = {
+            "out": False,
+            "hostname": "1.1.1.1",
+            "tablename": "sample",
+            "message": "Uncaught exception during get API call - please report:"
+            " \'{}\'".format(six.text_type(table))}
+        with patch("jnpr.junos.device.Device.execute") as mock_execute:
+            ret = junos.get_table(table, file)
+            self.assertEqual(ret["out"], ret_exp["out"])
+            self.assertEqual(ret["tablename"], ret_exp["tablename"])
+            self.assertEqual(ret["message"], ret_exp["message"])
+
+    def test_get_table_connect_closed_error(self):
+        table = "ModuleTable"
+        file = "inventory.yml"
+        ret_exp = {
+            "out": False,
+            "hostname": "1.1.1.1",
+            "tablename": "ModuleTable",
+            "message": "Got ConnectClosedError exception. Connection lost with Device(1.1.1.1)",
+        }
+        with patch("jnpr.junos.factory.optable.OpTable.get") as mock_load:
+            dev = Device(host="1.1.1.1", user="rick")
+            mock_load.side_effect = ConnectClosedError(dev)
+            ret = junos.get_table(table, file)
+            self.assertEqual(ret["out"], ret_exp["out"])
+            self.assertEqual(ret["tablename"], ret_exp["tablename"])
+            self.assertEqual(ret["message"], ret_exp["message"])
+
+    def test_get_table_inventory(self):
+        table = "ModuleTable"
+        file = "inventory.yml"
+        pyez_tables_path = os.path.dirname(os.path.abspath(tables_dir.__file__))
+        path = pyez_tables_path
+        with patch("jnpr.junos.device.Device.execute") as mock_execute, patch(
+            "salt.utils.json.dumps"
+        ) as mock_dumps:
+            mock_dumps.return_value = "json rpc reply"
+            m = mock_open()
+            ret = junos.get_table(table, file, path)
+            self.assertEqual(ret["out"], True)
+
+    def test_get_table_no_path_inventory(self):
+        table = "ModuleTable"
+        file = "inventory.yml"
+        with patch("jnpr.junos.device.Device.execute") as mock_execute, patch(
+            "salt.utils.json.dumps"
+        ) as mock_dumps:
+            mock_dumps.return_value = "json rpc reply"
+            m = mock_open()
+            ret = junos.get_table(table, file)
+            self.assertEqual(ret["out"], True)


Code review comments:
diff_hunk: @@ -33,16 +33,15 @@ def wrapper(*args, **kwargs):
 
 
 @resultdecorator
-def rpc(name, dest=None, format="xml", args=None, **kwargs):
+def rpc(name, **kwargs):
path: salt/states/junos.py
text: @user1:
changing an interface for a function which has been published without a deprecation cycle having been done is not allowed

@user2:
Salt initially made changes in Junos-salt modules https://github.com/saltstack/salt/commit/93ee5ee2b01b6f752f5538d0da413719d410d65c#diff-15ad308a12ea5b6d5188dd4a94eab4a0R490 https://github.com/saltstack/salt/commit/93ee5ee2b01b6f752f5538d0da413719d410d65c#diff-15ad308a12ea5b6d5188dd4a94eab4a0R400. This commit caused all our state module to break https://github.com/Juniper/salt/issues/77. 

@user1  Hi David, The issue was caused as salt changed positional arguments(*args) in our modules function definition to **kwargs. But, the state modules were not modified in those commit. we had changed it to **kwargs to keep it in line with changes done by salt. 
Also we have neither changes any function name of removed any parameters, just that they are consumed via **kwargs. 

@user1:
@user2 I have checked back as far as 2017.7.0 and the function signature is the same as that remove by your change. I can understand that this may not line up with Juniper's own functionality but it is an interface which has been published for a few years and it MUST go through a deprecation cycle before it can be changed.  And I understand that the kwargs can handle the changes but the positional arguments MUST remain until the deprecation cycle for an interface change has completed. Note can add a new interface and mark the old interface for deprecation.  Existing customers cannot have the rug pulled out from underneath them without time to adjust.

@user2:
@user1 I have reverted to old function definition itself.

diff_hunk: @@ -149,7 +160,7 @@ def facts():
 
 
 @timeoutDecorator
-def rpc(cmd=None, dest=None, **kwargs):
+def rpc(cmd=None, **kwargs):
path: salt/modules/junos.py
text: @user1:
changing an interface for a function which has been published without a deprecation cycle having been done is not allowed

@user2:
same as above

@user1:
see reply for salt/states/junos.py about deprecation cycle and changing interfaces

@user2:
@user1 I have reverted to old code itself.

diff_hunk: @@ -188,20 +181,19 @@ def rollback(name, id, **kwargs):
 
     """
     ret = {"name": name, "changes": {}, "result": True, "comment": ""}
-    ret["changes"] = __salt__["junos.rollback"](id, **kwargs)
+    ret["changes"] = __salt__["junos.rollback"](**kwargs)
     return ret
 
 
 @resultdecorator
-def diff(name, d_id):
+def diff(name, **kwargs):
path: salt/states/junos.py
text: @user1:
Changing an interface for a function which has been published without a deprecation cycle having been done is not allowed. You could extend the interface

    def diff(name, d_id, **kwargs) 

and do a versionadded, but the interface has to be backwards compatible unless a deprecation cycle is performed

@user2:
same as above, also to add

For diff, the documentation clearly stated that it expects "id" as the parameter name but the name of the argument was "d_id". This is a bug and we don't see a reason to support "d_id". Also, this will go inline to changes done at https://github.com/saltstack/salt/commit/93ee5ee2b01b6f752f5538d0da413719d410d65c#diff-15ad308a12ea5b6d5188dd4a94eab4a0R490


@user1:
see reply for salt/states/junos.py about deprecation cycle and changing interfaces
The label of a positional argument doesn't really matter here compared to a documentation typo/error, but the positional argument is required otherwise the interface is changing without a deprecation cycle for the interface

@user2:
@user1 I have reverted to the old code function definition itself.

diff_hunk: @@ -71,8 +81,9 @@ def __virtual__():
     else:
         return (
             False,
-            "The junos module could not be loaded: "
-            "junos-eznc or jxmlease or proxy could not be loaded.",
+            "The junos or dependent module could not be loaded: "
+            "junos-eznc or jxmlease or or yamlordereddictloader or "
path: salt/modules/junos.py
text: @user1:
Got two or, 'or or'

@user2:
Fixed via e99f1395389022cda1f3cee5bb7ee2d350c342f7

diff_hunk: @@ -158,15 +152,14 @@ def commit(name, **kwargs):
 
 
 @resultdecorator
-def rollback(name, id, **kwargs):
+def rollback(name, **kwargs):
path: salt/states/junos.py
text: @user1:
Changing an interface for a function which has been published without a deprecation cycle having been done is not allowed

@user2:
same as above

@user1:
see reply for salt/states/junos.py about deprecation cycle and changing interfaces



@user2:
@user1 I have reverted to old code itself.