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

Jump to bottom

roon-server: init at 100600401 #63551

Merged
merged 2 commits into from Jul 2, 2019
Merged

roon-server: init at 100600401 #63551

merged 2 commits into from Jul 2, 2019

Conversation

Steell
Copy link
Contributor

@Steell Steell commented Jun 20, 2019

Motivation for this change

I use this to organize and listen to my music. Also, I haven't seen much movement on PR #51308, so this addresses the comments left there.

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.

@Steell Steell requested a review from infinisil as a code owner June 20, 2019 04:18
@peterhoeg
Copy link
Member

Can we run this with DynamicUser = true; instead of assigning a system user?

@Steell
Copy link
Contributor Author

Steell commented Jun 21, 2019

@peterhoeg Done. I'm not super familiar with DynamicUser so let me know if I've missed anything.

peterhoeg
peterhoeg requested changes Jun 23, 2019
View reviewed changes
pkgs/servers/roon-server/default.nix Show resolved Hide resolved
pkgs/servers/roon-server/default.nix Outdated Show resolved Hide resolved
@peterhoeg
Copy link
Member

I'm not super familiar with DynamicUser so let me know if I've missed anything.

That looks pretty good - I don't use roon so I can't test it out but if it runs with DynamicUser, that's great.

One thing on the nixos module though - it's always a good idea to add an openFirewall (default to false) that opens the required ports.

@Steell
Copy link
Contributor Author

Steell commented Jun 24, 2019 •
edited

That looks pretty good - I don't use roon so I can't test it out but if it runs with DynamicUser, that's great.

It appears to be working fine on my end.

it's always a good idea to add an openFirewall (default to false) that opens the required ports.

Done.

@Steell
Copy link
Contributor Author

Steell commented Jun 24, 2019

@lovek323 FYI

@peterhoeg
Copy link
Member

@GrahamcOfBorg build roon-server

@peterhoeg peterhoeg mentioned this pull request Jun 25, 2019
Closed
10 tasks
@infinisil
Copy link
Member

Looking good to me

@peterhoeg peterhoeg merged commit 10dd03e into NixOS:master Jul 2, 2019
@peterhoeg
Copy link
Member

Thanks for your hard work and patience @steel and welcome to Nix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterhoeg peterhoeg peterhoeg requested changes

@JohnAZoidberg JohnAZoidberg JohnAZoidberg left review comments

@infinisil infinisil infinisil approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Steell @peterhoeg @infinisil @JohnAZoidberg @Janik-Haag