Commandline <> Lib communication using generators #37

gronke · 2017-08-27T22:02:09Z

implements #16

Specific class methods will allow to set a yields=True argument that will turn it into a Python generator. This generator will push status updates in form of libiocage.lib.events.*

Demo Snippets

This are some demos that demonstrate generators. They don't aim to be the exact output we will provide to the users.

Fetch: https://asciinema.org/a/CSpa7Nw9KMXrD4MAUo9TItNlI
List: https://asciinema.org/a/Azzsm7RBOdRCunwzQpcFcio3v
Bulk-Set: https://asciinema.org/a/8bUcbXAk3MPwfcOkq7RNoNNyG

igalic

first look

igalic · 2017-08-28T08:51:49Z

libiocage/cli/start.py

        except Exception:
+            raise
            exit(1)


i don't think raise and exit will do much taken together

I'm sorry, but this is necessary to see python stacktraces, so I have this enabled until I prepare the branch for the merge. The errors that get swallowed from exit(1) are structural ones, not feature failures, so they are useful for development only. Already told @skarekrow that I'll find a better solution for this, because it lead to quite some confusion. For example this one #35 (comment)

skarekrow · 2017-08-29T17:09:49Z

libiocage/lib/Release.py

@@ -332,7 +376,7 @@ def fetch_updates(self):
                            "Components"
                        ))
                        f.truncate()
-                        f.close()


Why are you removing this? I don't see it being used anywhere, this will result in leaking fd's

with is already taking care of that. See http://effbot.org/zone/python-with-statement.htm

I know how with works ;) I commented on that in a different ticket. I just forgot to click that little expand thing.

skarekrow · 2017-08-29T17:10:27Z

libiocage/lib/Release.py

+                if isinstance(event, libiocage.lib.events.IocageEvent):
+                    yield event
+                else:
+                    # the only non-IocageEvent is out return value


skarekrow · 2017-08-29T17:10:46Z

libiocage/lib/Logger.py


-        self.log(message, level="spam", jail=jail, indent=indent)
+        # ToDo: Handle redrawig of multiline entries with different line count


s/redrawig/redrawing/ Sorry, OCD!

skarekrow · 2017-08-29T17:11:02Z

libiocage/lib/Logger.py

+            f"\033[{delta}F",  # CPL - Cursor Previous Line
+            "\r",               # CR - Carriage Return
+            self._indent(log_entry.message, log_entry.indent),
+            "\033[K",           # EL - Erease in Line


s/Erease/Erase/

skarekrow · 2017-08-29T17:11:28Z

libiocage/lib/Logger.py

+        # ToDo: Handle redrawig of multiline entries with different line count
+
+        output = "".join([
+            #"\033[s",            # SCP - Save Cursor Position


If this is used (assuming you left a comment on it because it was useful), why is it commented out then?

I was playing with various techniques to alter already printed output. Didn't get it to work as expected, but it was the most promising solution when it comes to multi-line editing

skarekrow · 2017-08-29T17:11:40Z

libiocage/lib/Logger.py

+            self._indent(log_entry.message, log_entry.indent),
+            "\033[K",           # EL - Erease in Line
+            "\n" * delta
+            #"\033[s"            # RCP - Restore Cursor Position


skarekrow · 2017-08-29T17:11:46Z

libiocage/lib/Logger.py

+        ])
+
+        sys.stdout.write(output)
+        #print(log_entry.message)


Debug print

skarekrow · 2017-08-29T17:12:22Z

libiocage/lib/Logger.py

-             indent=0):
+    def screen(self, message, indent=0, **kwargs):
+        """
+        Screen does never get printed to log files


You can drop does for that sentence, then add a s to get

skarekrow · 2017-08-29T17:13:28Z

libiocage/lib/Jail.py

@@ -511,7 +552,7 @@ def _launch_jail(self):

    def _start_vimage_network(self):

-        self.logger.log("Starting VNET/VIMAGE", jail=self)
+        self.logger.debug("Starting VNET/VIMAGE", jail=self)


Why debug and not info?

The library is not supposed to print. Info/Log gets printed by default and we want all stdout to be generated from the event responses.

skarekrow · 2017-08-29T17:14:13Z

libiocage/cli/fetch.py

-#                                                 "values: basic, digest")
-# @click.option("--verify/--noverify", "-V/-NV", default=True,
-#               help="Enable or disable verifying SSL cert for HTTP fetching.")
-# def cli(url, files, release, update):


Why are we removing current CLI flags?

Because they were not implemented yet, so I removed it to clean up the file. Most probably #43 will cause need to re-apply and use this option. I've updated the CLI criteria for signature checking, so that it's not forgotten.

skarekrow

Another quick review :)

skarekrow · 2017-08-30T18:50:03Z

libiocage/cli/__init__.py

+            output += event.get_state_string(
+                done="OK",
+                error="failed",
+                skipped="skipped",


They should all be capitalized to keep with legacy's style. SKIPPED, FAILED.

skarekrow · 2017-08-30T18:52:46Z

libiocage/cli/__init__.py

 @click.command(cls=IOCageCLI)
-@click.version_option(version="0.9.10 07/30/2017", prog_name="iocage",
+@click.version_option(version="0.2.11 08/29/2017", prog_name="ioc",


Where are you getting this versioning scheme from? Besides our placeholder release, we haven't discussed versioning much.

I started with 0.0.1 and everytime it will introduce new features I'd love to bump the minor version. For breaking changes (after 1.0.0) the major version number will increase. We should discuss and document the versioning system!

skarekrow · 2017-08-30T18:59:56Z

libiocage/cli/list.py

 import libiocage.lib.Logger

+supported_output_formats = ['table', 'csv', 'list']


JSON soon? :D

Maybe now. That's super easy. But csv and list come with some nice optimizations - they stream their output. For JSON that would be useless :) But I will add it because it's a low hanging fruit!

Update: What about yaml?

yaml is even lower hanging fruit! It should also buy us XML iirc

no no no double no

@igalic I can understand that you want XML so badly, but we can't just disable all other list options just for that! @skarekrow would be very disappointed if we drop the table output!

skarekrow · 2017-08-30T19:02:04Z

libiocage/cli/list.py


-def _lookup_jail_value(jail, key):
-    if key in libiocage.lib.Jails.Jails.JAIL_KEYS:
+    # ToDo: Move this into lib/Jails ?


This I think better belongs in helpers

skarekrow · 2017-08-30T19:02:53Z

libiocage/cli/start.py

        except Exception:
+            failed_jails.append(jail)
+            continue
            exit(1)


exit after continue?

No exit! Was a left-over from the previous implementation. Will be cleaned with the next commit!

skarekrow · 2017-08-30T19:09:30Z

libiocage/lib/events.py

@@ -0,0 +1,275 @@
+from timeit import default_timer as timer


Missing iocage license

skarekrow · 2017-08-30T19:09:59Z

libiocage/lib/Logger.py

@@ -1,9 +1,38 @@
 import os


Missing iocage license

skarekrow · 2017-08-30T19:10:20Z

libiocage/lib/Jails.py

@@ -1,12 +1,13 @@
-import re
+from typing import Generator, Union, Iterable


Missing iocage license

skarekrow · 2017-08-30T19:10:56Z

libiocage/lib/Jails.py


 import libzfs

 import libiocage.lib.Jail
+import libiocage.lib.JailFilter
 import libiocage.lib.helpers


Are you running isort like CONTRIBUTING says? These look out of order, I'm seeing that around the files here in general

Didn't care about the order yet. Let's put in a static analyzer rule in addition to the contribution guidelines.

I suggest to tackle those kind of changes in a separate PR, so that we concentrate on a specific feature in a PR. Too often features have implications to other parts of libiocage as well. But I'm pretty sure this will change in the near future after the interfaces are well tested.

#47 is the ticket for that

skarekrow · 2017-08-30T19:12:59Z

libiocage/lib/Jails.py

-            return jails
+        self._filters = None
+        self.filters = filters
+        list.__init__(self, [])


Nope. Line 31 makes the _filters property available, the line below calls the filters property setter.

gronke · 2017-08-31T11:10:53Z

@skarekrow we can work on the license issue in a different PR :)

skarekrow · 2017-08-31T19:20:33Z

libiocage/lib/Jail.py

@@ -661,22 +711,21 @@ def _teardown_mounts(self):
                )

    def _resolve_name(self, text):
+
+        if (text is None) or (len(text)==0):


I know, you know I hate parenthesis, but I think this is a good example. It makes de-compacting that hard on my eyes. Instead of if test is None or len(text) == 0, it's easier to see those elements to me. I won't mark as needs changes, because it's your personal style ;)

But just felt I had to say it atleast! :D

skarekrow · 2017-08-31T19:35:42Z

libiocage/lib/Logger.py

@@ -85,7 +85,7 @@ def __init__(self, print_level=None, log_directory="/var/log/iocage"):

    @property
    def default_print_level(self):
-        return "spam"
+        return "info"


info now? You told me that printed, does it not now?

skarekrow · 2017-08-31T19:36:37Z

libiocage/lib/JailConfig.py

@@ -213,7 +213,7 @@ def _set_name(self, name, **kwargs):
            self["id"] = name
        else:
            try:
-                self["id"] = str(uuid.UUID(name)) # legacy support
+                self["id"] = str(uuid.UUID(name))  # legacy support


Since this is for legacy support, I think uuid would be a better indicator of what the id is. As id can be a bit ambiguous

The ID brings together all the concepts of name and uuid :) It's the only true identifier of a jail.

igalic

finally understood one of the errors that mypy is throwing at me

igalic · 2017-09-02T17:21:33Z

libiocage/lib/JailFilter.py

+                if self._validate_name_filter_string(filter_value) is False:
+                    raise libiocage.lib.errors.JailFilterInvalidName(
+                        filter_value,
+                        logger=self.logger


The class Term has no attribute logger, if it needs one, we should extend its __init__

there's now a patch for this #62

zachfi · 2017-09-05T23:24:11Z

libiocage/cli/list.py

 import libiocage.lib.Logger

+supported_output_formats = ['table', 'csv', 'list', 'json']


gronke force-pushed the chore/cli-generators branch from a2a5654 to 10e8c67 Compare August 27, 2017 22:03

gronke added the work in progress label Aug 27, 2017

igalic reviewed Aug 28, 2017

View reviewed changes

gronke force-pushed the chore/cli-generators branch 5 times, most recently from a51e1bd to 2da2fe6 Compare August 29, 2017 00:27

skarekrow suggested changes Aug 29, 2017

View reviewed changes

gronke force-pushed the chore/cli-generators branch 6 times, most recently from 87306eb to 21b6a29 Compare August 30, 2017 18:07

skarekrow suggested changes Aug 30, 2017

View reviewed changes

gronke force-pushed the chore/cli-generators branch 2 times, most recently from d5d02f0 to 150f500 Compare August 31, 2017 15:40

gronke mentioned this pull request Aug 31, 2017

Feature: Internationalize CLI output #49

Open

gronke force-pushed the chore/cli-generators branch from a87ce12 to 37e016f Compare August 31, 2017 17:15

skarekrow previously approved these changes Aug 31, 2017

View reviewed changes

gronke dismissed skarekrow’s stale review via 3d05e02 August 31, 2017 19:12

gronke added 7 commits August 31, 2017 19:14

cli-lib communication draft with generators

76ec958

follow generator event notification approach

7bd5d1e

apply generators to list

c9875a3

address review #pullrequestreview-59313901

57f9b74

fix code style

331679e

improve event stack and release events

7641528

globs for cli/start and cli/stop

23f6a70

gronke added 5 commits August 31, 2017 19:14

properly handle multi-line log entries for dynamic output editing

c767b5e

improve globs and cli/commands

bd635d8

cli/list can generate json output

4852eac

utilize helpers.validatate_name in JailConfig

107bc48

improve id/name handling

fb4c6d8

skarekrow previously approved these changes Aug 31, 2017

View reviewed changes

skarekrow reviewed Aug 31, 2017

View reviewed changes

gronke dismissed skarekrow’s stale review via 18e4db1 August 31, 2017 19:25

gronke force-pushed the chore/cli-generators branch from 3d05e02 to 18e4db1 Compare August 31, 2017 19:25

refactor cli/start exit

18e4db1

skarekrow reviewed Aug 31, 2017

View reviewed changes

gronke added 2 commits August 31, 2017 22:54

fix typo in errors.JailUnknownIdentifier

58be926

fix an error showing up in the logs when no custom hostname was set

6d92b92

gronke removed the work in progress label Aug 31, 2017

refactoring generators branch

70a8a4e

skarekrow approved these changes Aug 31, 2017

View reviewed changes

gronke merged commit dcfc41e into master Aug 31, 2017

gronke deleted the chore/cli-generators branch August 31, 2017 23:59

igalic mentioned this pull request Sep 1, 2017

implement type-hints for all our functions #42

Closed

igalic reviewed Sep 2, 2017

View reviewed changes

zachfi reviewed Sep 5, 2017

View reviewed changes


		self.log(message, level="spam", jail=jail, indent=indent)
		# ToDo: Handle redrawig of multiline entries with different line count

		import libiocage.lib.Logger

		supported_output_formats = ['table', 'csv', 'list']

		@@ -1,12 +1,13 @@
		import re
		from typing import Generator, Union, Iterable

Commandline <> Lib communication using generators #37

Commandline <> Lib communication using generators #37

Uh oh!

Conversation

gronke commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Demo Snippets

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skarekrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gronke Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gronke commented Aug 27, 2017 •

edited

Loading

gronke Aug 30, 2017 •

edited

Loading