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

nixos/tox-node: init #59368

Merged
merged 2 commits into from Apr 15, 2019
Merged

nixos/tox-node: init #59368

merged 2 commits into from Apr 15, 2019

Conversation

suhr
Copy link
Contributor

@suhr suhr commented Apr 12, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Right now it uses the same uid as tox-bootstrapd, this probably should be changed.

nixos/modules/services/networking/tox-node.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/tox-node.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/tox-node.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/tox-node.nix Outdated Show resolved Hide resolved
@suhr suhr force-pushed the tox-node branch 2 times, most recently from fdbfe47 to 3569d48 Compare April 12, 2019 20:31
cfg = config.services.tox-node;
homeDir = "/var/lib/tox-node";

bootstrapNodes = [
Copy link
Member

Choose a reason for hiding this comment

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

Where is this list coming from? Is it possible to pull it from the tox-node package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From here: https://github.com/tox-rs/tox-node/blob/master/dpkg/config.yml

Is it possible to pull it from the tox-node package?

It is easier to add them into the module.

Copy link
Member

Choose a reason for hiding this comment

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

@adisbladis does this answer satisfy you?

Copy link
Member

Choose a reason for hiding this comment

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

I'm reluctant to merge this PR as-is. I don't like the additional maintenance burden put on maintainers while updating the package.
A lot of our users are not on NixOS and will possibly miss updating the service.

It's not a lot of extra lines of code to pull this data from the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to actually maintain this module.

It's not a lot of extra lines of code to pull this data from the package.

It's not very clear how to do this. Would you like to explain how to pull node list from the package?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another possibility is to reconstitute the config out-of-store at runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but does it make sense to have this as an option so the user can override

I believe so.

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 think we would need IFD at all.
I'll hack a POC of what I have in mind :)

Copy link
Member

Choose a reason for hiding this comment

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

Check out my changes in my nixpkgs fork: https://github.com/adisbladis/nixpkgs/tree/tox-node (adisbladis@984461c).

I noticed that the dpkg directory is not in tox-node-0.0.7 yet so I had to resort to using fetchurl on this file separately for now.

Feel free to cherry-pick (and optionally squash) this change if you are satisfied with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to cherry-pick (and optionally squash) this change if you are satisfied with it.

Done.

@suhr suhr force-pushed the tox-node branch 2 times, most recently from c931726 to 967a1cd Compare April 13, 2019 11:54
@suhr
Copy link
Contributor Author

suhr commented Apr 14, 2019

r?

@aanderse
Copy link
Member

r?

You'll have to elaborate on that.

@suhr
Copy link
Contributor Author

suhr commented Apr 14, 2019

Github says “changes required” even though I've applied necessary changes.

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

In general LGTM

I'd like to hear back from @adisbladis on the bootstrapNodes first, though.
Bonus points if anyone else who uses tox-node could test this as well.

@adisbladis
Copy link
Member

Aside from my notes on hard-coded bootstrap nodes this PR looks fine.
A changelog entry for 19.09 would be great too!

suhr and others added 2 commits April 15, 2019 09:27
Get these from upstream tox-node package instead.
This is likely to cause less maintenance overhead over time and
following upstream bootstrap node changes is automated.
@adisbladis
Copy link
Member

Looks great now! Thanks for your contribution :)

@suhr suhr deleted the tox-node branch April 15, 2019 10:05
@samueldr
Copy link
Member

samueldr commented Apr 15, 2019

This PR broke tarball build due to the missing descriptions on the module options.

(Sorry if the tone seems a bit rough, just stating what I found out.)


Adding more details, in case it confuses people:

The tarball build is more strict and will abort the build when any warning is produced. I do not know if there are easier methods to catch these warnings other than doing the tarball build.

@joachifm
Copy link
Contributor

The grahamcofborg-eval-nixpkgs-tarball was reported as successful, is that the same thing? If so, why the discrepancy in outcome?

@samueldr
Copy link
Member

@joachifm since it happens at build time no, ofborg doesn't build untrusted inputs automatically, only evals untrusted inputs automatically.

@adisbladis
Copy link
Member

Whoops.. Descriptions added in 9a176d6

@samueldr
Copy link
Member

Thanks @adisbladis!

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

Successfully merging this pull request may close these issues.

None yet

7 participants