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

Expose resources to deployment.keys #1054

Closed
wants to merge 1 commit into from

Conversation

tomberek
Copy link
Contributor

@tomberek tomberek commented Nov 23, 2018

Re-evaluate info.machines in order to expose resources and publicIPv4 to deployment.keys.
Addresses #959

There may be a better way to do this, this is merely the first thing I was able to make work. Some defensive programming is needed due to the duplication evaluation, so something like this:

my_ip = let
     net = nodes.my-machine.config.networking;
     res = builtins.tryEval (
     if net.publicIPv4 != null then net.publicIPv4 else
     if net.privateIPv4 != null then net.privateIPv4 else "");
   in
    if res.success then res.value else "error";
...
deployment.keys."ip".text = my_ip;

Re-evaluate info.machines in order to expose resources and publicIPv4 to
deployment.keys.
@singron
Copy link

singron commented Jun 9, 2019

Is there anything blocking this? I ran into this issue and it would be nice to have a solution.

@dsg22
Copy link
Contributor

dsg22 commented Oct 16, 2019

I have a similar problem with commandOutput resources not being available during the initial deployment. Sadly, this patch does not make them available.

In the longer term I think we need a more complete solution to these issues (shouldn't these resources be evaluated lazily?). This patch might be an interim partial solution until then.

@RaitoBezarius
Copy link
Member

Heads up, can we do anything to help core contributors to merge this or @tomberek can we help you to clean it?

@tomberek
Copy link
Contributor Author

This change is fairly simple, but probably could be done cleaner or in a way that doesn’t require the tryEval construct. Or it’s done for the user behind the scenes.

@AmineChikhaoui
Copy link
Member

I'm still not sure it would be a good idea to have 3 evals now, 2 evals is already slow and expensive depending on the number of machines in the network.

@edolstra btw would nix flakes make any of this better ? I hear there would be eval caching but also I'm not up to speed with the flakes branch of nixops maybe it makes some things better ?

@tomberek
Copy link
Contributor Author

I agree that the implementation isn’t ideal, I just don’t know if of an alternative. Consider this implementation just a PoC to test if the feature is desirable/useful.

@grahamc
Copy link
Member

grahamc commented Mar 26, 2020

Hello!

Thank you for this PR.

In the past several months, some major changes have taken place in
NixOps:

  1. Backends have been removed, preferring a plugin-based architecture.
    Here are some of them:

  2. NixOps Core has been updated to be Python 3 only, and at the
    same time, MyPy type hints have been added and are now strictly
    required during CI.

This is all accumulating in to what I hope will be a NixOps 2.0
release
. There is a tracking issue for that:
#1242 . It is possible that
more core changes will be made to NixOps for this release, with a
focus on simplifying NixOps core and making it easier to use and work
on.

My hope is that by adding types and more thorough automated testing,
it will be easier for contributors to make improvements, and for
contributions like this one to merge in the future.

However, because of the major changes, it has become likely that this
PR cannot merge right now as it is. The backlog of now-unmergable PRs
makes it hard to see which ones are being kept up to date.

If you would like to see this merge, please bring it up to date with
master and reopen it
. If the or mypy type checking fails, please
correct any issues and then reopen it. I will be looking primarily at
open PRs whose tests are all green.

Thank you again for the work you've done here, I am sorry to be
closing it now.

Graham

@grahamc grahamc closed this Mar 26, 2020
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.

6 participants