[RFR] Added ssh credentials handling #30

grafuls · 2018-06-29T18:56:05Z

No description provided.

grafuls · 2018-06-29T19:24:32Z

jan-zmeskal · 2018-07-10T14:32:57Z

disruption_generator/cli.py

 @click.version_option(version=__version__)
-def main(experiments_path):
+def main(experiments_path, ssh_host_key):


Maybe I am missing something there. But main here is defined with two args that have no default value. However, in the end of the program it is called with no args provided.

These values are being taken from the click decorator which does state the default values for those.

jan-zmeskal · 2018-07-10T14:39:46Z

disruption_generator/cli.py

    except (OSError, asyncssh.Error) as exc:
        sys.exit("SSH connection failed: " + str(exc))


-async def execute(experiments_path):
+async def execute(experiments_path, ssh_host_key):
+    ssh_host_key = [ssh_host_key] if ssh_host_key else ssh_host_key


I cannot wrap my head around this construction. ssh_host_key has no default value, there you cannot really call execute without specifying it. I suppose you could specify False as ssh_host_key value but that's probably not how it's supposed to work. Given that, it seems that else clause should never happen.

ssh_host_key = ssh_host_key or [ssh_host_key]

jan-zmeskal · 2018-07-10T14:42:06Z

disruption_generator/cli.py

    _files = []
    for (dirpath, dirnames, filenames) in walk(experiments_path):
-        for _file in filenames:
-            _files.append(path.join(dirpath, _file))
+        _files = [


Here we could maybe log files that are excluded (do not end with .yml/.yaml) as warning.

jan-zmeskal · 2018-07-10T14:44:37Z

disruption_generator/cli.py

@@ -60,11 +78,14 @@ def main(experiments_path):

            if result:
                click.echo("Triggering: %s" % action.name)
-                trigger = Trigger(action)
+                _username = action.username if action.username else "root"


This behavior (using root if no username is specifie) should be exposed at some higher level.

jan-zmeskal · 2018-07-10T14:52:10Z

disruption_generator/listener/alistener.py

            logger.debug("Connected to %s", self.hostname)
-            stdin, stdout, stderr = await conn.open_session("tail -F %s" % filepath)
+            stdin, stdout, stderr = await conn.open_session(
+                "tail -F %s" % filepath


Let's please stick with new-style string formatting: "tail -F {}".format(filepath).

jan-zmeskal · 2018-07-10T14:53:44Z

disruption_generator/parsers/experiment_parser.py

+                return _listener
+            except KeyError as ex:
+                raise ParserException(
+                    "Missing %s definition from listener section", ex


Pls new-style string formatting.

jan-zmeskal · 2018-07-10T14:59:54Z

disruption_generator/parsers/experiment_parser.py

+                    wait = trigger[key][config.WAIT_KEY]
+                    timeout = trigger[key][config.TIMEOUT_KEY]
+
+                    trigger = Action(


Why not call it action? This naming seems kind of confusing since trigger (at least according to example.yaml) is higher-category term than action.

jan-zmeskal · 2018-07-10T15:02:53Z

disruption_generator/parsers/experiment_parser.py

@@ -89,15 +115,23 @@ def _init_actions(element_trigger):

        _scenarios = []
        for disrupt_action in doc:
-            name = disrupt_action[config.DISRUPT_ACTION_STR][0][config.NAME_STR]
+            name = disrupt_action[config.DISRUPT_ACTION_KEY][0][


Why is there [0] index? In example.yaml, name key is directly under disrupt_action.

Cause it bring a 1 element list

jan-zmeskal · 2018-07-10T15:10:14Z

disruption_generator/trigger/trigger.py

-            ALL_ACTIONS
+        attr_err_msg = (
+            "Unexpected disruptive action other than %s"
+            % ", ".join(ALL_ACTIONS)


Please use new-style string formatting.

StLuke · 2018-07-11T12:21:58Z

disruption_generator/cli.py

    except (OSError, asyncssh.Error) as exc:
        sys.exit("SSH connection failed: " + str(exc))


-async def execute(experiments_path):
+async def execute(experiments_path, ssh_host_key):
+    ssh_host_key = [ssh_host_key] if ssh_host_key else ssh_host_key


ssh_host_key = ssh_host_key or [ssh_host_key]

StLuke · 2018-07-11T12:22:27Z

disruption_generator/cli.py

                try:
                    disruption = getattr(trigger, action.name)
                except AssertionError as err:
-                    logger.info(err)
+                    logger.error(err)
                    return 1


would be nice to have return values documented somewhere in config file

StLuke · 2018-07-11T12:23:08Z

disruption_generator/parsers/config.py

-NAME_STR = "name"
-LISTENER_STR = "listener"
-TRIGGER_STR = "trigger"
+ACTION_KEY = "action"


I dont get the _KEY

This is what parses should be looking for in experiment definition file, e.g. this one: https://github.com/grafuls/disruption_generator/blob/master/disruption_generator/experiments/example.yaml

_KEY as in dictionary for key, value.

StLuke · 2018-07-11T12:23:47Z

disruption_generator/parsers/experiment_parser.py

+                host = element_listener[config.HOST_KEY]
+                username = element_listener[config.USERNAME_KEY]
+                password = element_listener[config.PASSWORD_KEY]
+                _listener = Listener(


shouldnt this be after except branch? dont think you are testing for KeyError there

grafuls · 2018-07-17T08:42:40Z

@jan-zmeskal @StLuke Please review additional changes based on your comments.

Added ssh credentials handling

850c510

grafuls added the enhancement New feature or request label Jun 29, 2018

grafuls added this to the POC milestone Jun 29, 2018

grafuls self-assigned this Jun 29, 2018

grafuls requested review from KwisatzHaderach, petr-balogh, jan-zmeskal and ILpinto June 29, 2018 18:56

Modified key option from -s to -k

fab6f0e

Added username and password SSH capabilities

171a0dd

ILpinto approved these changes Jul 3, 2018

View reviewed changes

jan-zmeskal reviewed Jul 10, 2018

View reviewed changes

jan-zmeskal mentioned this pull request Jul 11, 2018

[RFR] Splitting unit tests into distinct modules #41

Merged

StLuke reviewed Jul 11, 2018

View reviewed changes

jan-zmeskal mentioned this pull request Jul 11, 2018

[Bug] Missing tests #34

Open

PR comments amended

4e52979

grafuls changed the title ~~Added ssh credentials handling~~ [RFR] Added ssh credentials handling Jul 13, 2018

jan-zmeskal approved these changes Jul 17, 2018

View reviewed changes

grafuls merged commit 66a67c0 into master Jul 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFR] Added ssh credentials handling #30

[RFR] Added ssh credentials handling #30

grafuls commented Jun 29, 2018

grafuls commented Jun 29, 2018

jan-zmeskal Jul 10, 2018

grafuls Jul 12, 2018

jan-zmeskal Jul 10, 2018

StLuke Jul 11, 2018

jan-zmeskal Jul 10, 2018

grafuls Jul 12, 2018

jan-zmeskal Jul 10, 2018

jan-zmeskal Jul 10, 2018

jan-zmeskal Jul 10, 2018

jan-zmeskal Jul 10, 2018

jan-zmeskal Jul 10, 2018

grafuls Jul 13, 2018

jan-zmeskal Jul 10, 2018

StLuke Jul 11, 2018

StLuke Jul 11, 2018

StLuke Jul 11, 2018

jan-zmeskal Jul 11, 2018

grafuls Jul 12, 2018

StLuke Jul 11, 2018

grafuls commented Jul 17, 2018

[RFR] Added ssh credentials handling #30

[RFR] Added ssh credentials handling #30

Conversation

grafuls commented Jun 29, 2018

grafuls commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafuls commented Jul 17, 2018