Move from xml intermediate Nix representation to JSON#1275
Move from xml intermediate Nix representation to JSON#1275grahamc merged 13 commits intoNixOS:masterfrom
Conversation
grahamc
left a comment
There was a problem hiding this 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?
fbb2d63 to
34936a1
Compare
874d179 to
9f03c29
Compare
|
lgtm :) |
|
This is great! I think the only thing it needs from here is a bit of docs for a migration guide |
b5b37cb to
8914d44
Compare
87f71e2 to
a74e555
Compare
grahamc
left a comment
There was a problem hiding this comment.
lookin' real fancy. Some questions, and I'm giving this a clone to try it out!
|
Nice, when causing a developer-error in the type of a module option: |
|
Not sure if this is from thi sbranch or on master already, but: Will look! |
|
I'm not getting that write error on master. It seems to come down to this: nixops/nixops/backends/__init__.py Lines 287 to 296 in 6f4f4da 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. |
|
This is why: By default, |
|
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. |
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 <[email protected]>
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 <[email protected]>
grahamc
left a comment
There was a problem hiding this comment.
lgtm, the ratchet is unhappy because the generic validation code operates on Any which ... yep.
|
woot: |
|
Can we create a branch 'pre-xml-to-json' to keep the plugins alive before working on the switch? |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
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.