Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move from xml intermediate Nix representation to JSON #1275

Merged
merged 13 commits into from Apr 23, 2020

Conversation

@adisbladis
Copy link
Member

adisbladis commented Apr 2, 2020

This change is intended to make life easier for plugin authors.
We have removed the XML parameter to ResourceDefinition and you are
now only provided with a name & a dict containing the evaluated
values.

Note that this will break all current plugins.

An example of this in use in a plugin can be found here: https://github.com/adisbladis/nixops-terraform.

@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch from 757c225 to d8249f9 Apr 2, 2020
@adisbladis adisbladis mentioned this pull request Apr 2, 2020
12 of 16 tasks complete
@grahamc grahamc force-pushed the adisbladis:xml-to-json branch from d8249f9 to abb053e Apr 8, 2020
Copy link
Member

grahamc left a comment

One thing I'm noticing, though, is the old code had some type information which the new code doesn't, and maybe we should do some type hinting and type conversions?

Also, I think it'd be useful to write down some docs, like that the config passed to a MachineDefinition is the deployment attrset for the node, and also how to convert nixops1 plugins to this new mechanism for nixops2. Maybe next to the authoring doc, as a updating to NixOps 2 doc?

nixops/backends/__init__.py Outdated Show resolved Hide resolved
nixops/backends/__init__.py Show resolved Hide resolved
@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch from 9d07c64 to ebcc849 Apr 15, 2020
nixops/backends/__init__.py Outdated Show resolved Hide resolved
@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch 2 times, most recently from fbb2d63 to 34936a1 Apr 15, 2020
nixops/util.py Outdated Show resolved Hide resolved
@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch 2 times, most recently from 874d179 to 9f03c29 Apr 15, 2020
@grahamc
Copy link
Member

grahamc commented Apr 15, 2020

lgtm :)

@grahamc
Copy link
Member

grahamc commented Apr 15, 2020

This is great! I think the only thing it needs from here is a bit of docs for a migration guide

@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch 3 times, most recently from b5b37cb to 8914d44 Apr 16, 2020
nixops/resources/__init__.py Outdated Show resolved Hide resolved
@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch 3 times, most recently from 87f71e2 to a74e555 Apr 16, 2020
Copy link
Member

grahamc left a comment

lookin' real fancy. Some questions, and I'm giving this a clone to try it out!

doc/plugins/authoring.rst Outdated Show resolved Hide resolved
doc/plugins/authoring.rst Outdated Show resolved Hide resolved
doc/plugins/authoring.rst Show resolved Hide resolved
nixops/backends/__init__.py Outdated Show resolved Hide resolved
@grahamc
Copy link
Member

grahamc commented Apr 17, 2020

Nice, when causing a developer-error in the type of a module option:

TypeError: type of hasFastConnection must be bool; got str instead
@grahamc
Copy link
Member

grahamc commented Apr 17, 2020

Not sure if this is from thi sbranch or on master already, but:

kif.........> uploading key ‘vault-login’...
kif.........> error: Traceback (most recent call last):
  File "/home/grahamc/projects/github.com/NixOS/nixops/nixops/deployment.py", line 910, in worker
    m.send_keys()
  File "/home/grahamc/projects/github.com/NixOS/nixops/nixops/backends/__init__.py", line 275, in send_keys
    f.write(opts["text"])
TypeError: write() argument must be str, not None

Will look!

@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch from a74e555 to ed5d566 Apr 20, 2020
@grahamc grahamc added this to the 2.0 milestone Apr 20, 2020
@grahamc
Copy link
Member

grahamc commented Apr 20, 2020

I'm not getting that write error on master. It seems to come down to this:

if "text" in opts:
with open(tmp, "w+") as f:
f.write(opts["text"])
elif "keyFile" in opts:
self._logged_exec(["cp", opts["keyFile"], tmp])
else:
raise Exception(
"Neither 'text' or 'keyFile' options were set for key '{0}'.".format(
k
)

I'm seeing this json come through with a null text field:

{
  "machines": {
    "kif": {
      "keys": {
        "xxx": {
          "_module": {
            "args": {
              "name": "xxx"
            },
            "check": true
          },
          "destDir": "/run/keys",
          "group": "root",
          "keyFile": "/home/grahamc/projects/github.com/xxx",
          "path": "/run/keys/xxx",
          "permissions": "0600",
          "text": null,
          "user": "root"
        },

the XML output:

<?xml version='1.0' encoding='utf-8'?>
<expr>
  <attrs>
    <attr column="5" line="118" name="machines" path="/home/grahamc/projects/github.com/NixOS/nixops/nix/eval-machine-info.nix">
      <attrs>
        <attr name="kif">
          <attrs>
            <attr column="40" line="121" name="keys" path="/home/grahamc/projects/github.com/NixOS/nixops/nix/eval-machine-info.nix">
              <attrs>
                <attr name="xxx">
                  <attrs>
                    <attr name="_module">
                      <attrs>
                        <attr name="args">
                          <attrs>
                            <attr name="name">
                              <string value="xxx" />
                            </attr>
                          </attrs>
                        </attr>
                        <attr name="check">
                          <bool value="true" />
                        </attr>
                      </attrs>
                    </attr>
                    <attr name="destDir">
                      <string value="/run/keys" />
                    </attr>
                    <attr name="group">
                      <string value="root" />
                    </attr>
                    <attr name="keyFile">
                      <path value="/home/grahamc/projects/github.com/xxx" />
                    </attr>
                    <attr name="path">
                      <string value="/run/keys/xxx" />
                    </attr>
                    <attr name="permissions">
                      <string value="0600" />
                    </attr>
                    <attr name="text">
                      <null />
                    </attr>
                    <attr name="user">
                      <string value="root" />
                    </attr>
                  </attrs>
                </attr>

so these seem to be the same, so I'm a bit confused.

@grahamc
Copy link
Member

grahamc commented Apr 20, 2020

This is why:

https://github.com/NixOS/nixops/pull/1275/files#diff-6ea77fd00d5f092f8150c2398eaf53d8L48-L49

By default, Nones would not get assigned in the first place.

@grahamc
Copy link
Member

grahamc commented Apr 20, 2020

I tried adding a type to the keys:

diff --git a/nixops/backends/__init__.py b/nixops/backends/__init__.py
index 4428de77..127107b1 100644
--- a/nixops/backends/__init__.py
+++ b/nixops/backends/__init__.py
@@ -8,6 +8,14 @@ import nixops.util
 import nixops.resources
 import nixops.ssh_util

+class KeyOptions(nixops.resources.ResourceOptions):
+    text: Optional[str]
+    keyFile: Optional[str]
+    destDir: str
+    user: str
+    group: str
+    permissions: str
+    fizz: str

 class MachineOptions(nixops.resources.ResourceOptions):
     storeKeysOnMachine: bool
@@ -15,7 +23,7 @@ class MachineOptions(nixops.resources.ResourceOptions):
     alwaysActivate: bool
     owners: Sequence[str]
     hasFastConnection: bool
-    keys: Mapping[str, Mapping]
+    keys: Mapping[str, KeyOptions]
     nixosRelease: str


@@ -29,7 +37,7 @@ class MachineDefinition(nixops.resources.ResourceDefinition):
     always_activate: bool
     owners: List[str]
     has_fast_connection: bool
-    keys: Mapping[str, Mapping]
+    keys: Mapping[str, KeyOptions]

     def __init__(self, name: str, config: nixops.resources.ResourceEval):
         super().__init__(name, config)
@@ -38,7 +46,8 @@ class MachineDefinition(nixops.resources.ResourceDefinition):
         self.always_activate = config["alwaysActivate"]
         self.owners = config["owners"]
         self.has_fast_connection = config["hasFastConnection"]
-        self.keys = config["keys"]
+        self.keys = {k: KeyOptions(**v) for k,v in config["keys"].items()}
+


 class MachineState(nixops.resources.ResourceState):
@@ -255,13 +264,14 @@ class MachineState(nixops.resources.ResourceState):
             return
         if self.store_keys_on_machine:
             return
+        opts: KeyOptions
         for k, opts in self.get_keys().items():
+            from pprint import pprint
+            pprint(opts)
             self.log("uploading key ‘{0}’...".format(k))
             tmp = self.depl.tempdir + "/key-" + self.name
-            if "destDir" not in opts:
-                raise Exception("Key '{}' has no 'destDir' specified.".format(k))

-            destDir = opts["destDir"].rstrip("/")
+            destDir = opts.destDir.rstrip("/")
             self.run_command(
                 (
                     "test -d '{0}' || ("
@@ -270,11 +280,11 @@ class MachineState(nixops.resources.ResourceState):
                 ).format(destDir)
             )

-            if "text" in opts:
+            if opts.text is not None:
                 with open(tmp, "w+") as f:
-                    f.write(opts["text"])
-            elif "keyFile" in opts:
-                self._logged_exec(["cp", opts["keyFile"], tmp])
+                    f.write(opts.text)
+            elif opts.keyFile is not None:
+                self._logged_exec(["cp", opts.keyFile, tmp])
             else:
                 raise Exception(
                     "Neither 'text' or 'keyFile' options were set for key '{0}'.".format(
@@ -306,7 +316,7 @@ class MachineState(nixops.resources.ResourceState):
                         "chmod '{3}' {0}",
                     ]
                 ).format(
-                    tmp_outfile_esc, opts["user"], opts["group"], opts["permissions"]
+                    tmp_outfile_esc, opts.user, opts.group, opts.permissions
                 )
             )
             self.run_command("mv " + tmp_outfile_esc + " " + outfile_esc)

I was surprised that mypy was totally happy with this, but it doesn't actually work or validate the data. I think this is what you were saying w.r.t. checking generics. Now I'm curious about if this is solved elsewhere, like by pydantic? I'd really like to be able to have typed mappings.

@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch from 7415fb8 to 9bf2cb4 Apr 21, 2020
@grahamc grahamc added this to In progress in kanban Apr 23, 2020
adisbladis and others added 12 commits Apr 2, 2020
This change is intended to make life easier for plugin authors.
We have removed the XML parameter to ResourceDefinition and you are
now only provided with a name & a dict containing the evaluated
values.
With nix-instantiate --xml, the output of evaluation of a path shows
the original path to the file. With --json, the output shows the path
if it were copied to the Nix store.

Applying toString in the expression forces Nix to skip copying it to
the store under any circumstance.
It's no longer required since moving to JSON representation.
Co-authored-by: Adam Höse <adam.hose@tweag.io>
Fix assertion to check for empty string.
Also fix type checking in ImmutableValidatedObject even when an
attribute is _not_ passed to the constructor.

Co-authored-by: Graham Christensen <graham.christensen@tweag.io>
@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch from 9bf2cb4 to 4444f5f Apr 23, 2020
Copy link
Member

grahamc left a comment

lgtm, the ratchet is unhappy because the generic validation code operates on Any which ... yep.

…key(s)
@adisbladis adisbladis force-pushed the adisbladis:xml-to-json branch from 27c9349 to 85faae0 Apr 23, 2020
@grahamc
Copy link
Member

grahamc commented Apr 23, 2020

woot:

Imprecision went up:
nixops.util:		17.62 -> 18.2
Imprecision went down:
nixops.backends:		38.92 -> 37.24
nixops.backends.none:		54.7 -> 50.0
nixops.deployment:		17.55 -> 16.32
nixops.resources:		39.53 -> 36.42
nixops.resources.commandOutput:		19.47 -> 17.5
nixops.resources.ssh_keypair:		48.33 -> 44.26
Total:		25.3 -> 24.6
@grahamc grahamc merged commit 9962fe4 into NixOS:master Apr 23, 2020
5 of 6 checks passed
5 of 6 checks passed
parsing
Details
build
Details
black
Details
mypy
Details
mypy-ratchet mypy-ratchet
Details
coverage
Details
kanban automation moved this from In progress to Done Apr 23, 2020
@tewfik-ghariani
Copy link
Contributor

tewfik-ghariani commented May 5, 2020

Can we create a branch 'pre-xml-to-json' to keep the plugins alive before working on the switch?

@nixos-discourse
Copy link

nixos-discourse commented May 11, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-3/7154/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
kanban
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.