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

add nixops eval subcommand #1319

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

manveru
Copy link
Contributor

@manveru manveru commented Apr 24, 2020

This lets you pass a file to eval to nixops eval.
With nixops eval you can evaluate nix code that has access to your deployment, so you can implement tooling around nixops that depends on your actual configuration easier. This is currently not possible using just nixops show-option, since that only accesses a single attribute.

The simplest example would be to get the list of nodes:

Given this eval.nix file:

{ nodes, ... }: builtins.attrNames nodes
nixops eval --json ./eval.nix
# => [ "node1", "node2", "node3" ]

A more complex example would be generating keys that nixops depends on, using a custom generator attribute on the key:

{ nodes, defaults, ... }:

let
  inherit (defaults.nixpkgs.pkgs) lib;
  inherit (lib)
    pipe filterAttrs mapAttrsToList concatStrings flatten attrValues
    optionalString all concatMapStrings;
in pipe nodes [
  (mapAttrsToList (name: config: config.deployment.keys))
  (map attrValues)
  flatten
  (concatMapStrings ({ keyFile, generator, ... }:
    optionalString (all (x: x != null) [ keyFile generator ]) ''
      set -e

      keyFile=${toString keyFile}
      if ! [[ -f $keyFile ]]; then
        1>&2 printf "Generating $(basename $keyFile)... "
        (${generator}) > $keyFile && 1>&2 echo "done" || (1>&2 echo "failed!" && exit 1)
      fi
    ''))
]

There are many more possibilities, but this really is a lot simpler than having to figure out the correct syntax for eval-machine-info yourself.

@grahamc
Copy link
Member

grahamc commented Apr 24, 2020

Before I review this, a few things: can you fixup the failures with black, and take a look to see if you can fix the ratchet's complaints about reduced type coverage?

And also if you could write a bit about what this is useful for, like what use cases you have for it -- etc. that'd be helpful!

@manveru
Copy link
Contributor Author

manveru commented Apr 24, 2020

Yeah... working on it :)

Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some feedback focusing mostly on the help messages. One thing this does is it sort of implicitly makes the structure of the evaluation's results an API. I know you already treat it like an API, but NixOps doesn't really make a claim about that right now.

I am not sure if we should do that, but want to still let you do what you need to do.

My thinking is two-fold:

  1. having a standard way for a NixOps network to become hydra-buildable is a great idea (Make networks buildable by Hydra #1320)
  2. if your goal is to have pre-deploy steps, then maybe a plugin and hooks would be a better approach, so a nixops deploy is doing the right thing?

I'm especially curious about your keyfile code there :)

what do you think?

@@ -527,6 +527,16 @@
help="include the physical specification in the evaluation",
)

subparser = add_subparser(
subparsers, "eval", help="eval the given file is nix code in the network expression"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
subparsers, "eval", help="eval the given file is nix code in the network expression"
subparsers, "eval", help="evaluate a Nix expression with the NixOps network as arguments"

my proposed wording isn't quite right, but maybe something like this?

subparsers, "eval", help="eval the given file is nix code in the network expression"
)
subparser.set_defaults(op=op_eval)
subparser.add_argument("code", metavar="CODE", help="code")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
subparser.add_argument("code", metavar="CODE", help="code")
subparser.add_argument("file", metavar="FILE", help="File containing a Nix expression")

subparser.set_defaults(op=op_eval)
subparser.add_argument("code", metavar="CODE", help="code")
subparser.add_argument(
"--json", action="store_true", help="print the option value in JSON format"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"--json", action="store_true", help="print the option value in JSON format"
"--json", action="store_true", help="print the resulting value as an JSON representation of the abstract syntax tree"

what does it print without --json?

subparser.add_argument(
"--json", action="store_true", help="print the option value in JSON format"
)
subparser.add_argument("--strict", action="store_true", help="enable strict evaluation")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a specific suggestion, but we might want to crib what the nix-instantiate manual says:

recursively evaluate list elements and attributes. Normally, such sub-expressions are left unevaluated
(since the Nix expression language is lazy).
Warning
This option can cause non-termination, because lazy data structures can be infinitely large.

I wonder why someone wouldn't use --strict --json? Should --json imply --strict?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, --json will only work with --strict if it's more than one level deep.

The use-case here was if you wanted to evaluate something but finely control the level of strictness and output Nix instead of JSON. For that you'd want lazy evaluation of functions, which is impossible if --strict is hardcoded.

Given that this is supposed to be used by other tooling (that's why I don't pass a plain string but a file instead), it shouldn't be too confusing or hard to configure it for the specific usage.

@@ -893,6 +893,14 @@ def op_show_option(args):
)


def op_eval(args):
with deployment(args) as depl:
depl.evaluate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily to be fixed here, but it would be nice to think of a way for the deployment API to not require being careful about calling evaluate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants