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.

Sign up for GitHub

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

nixos/modules: services.syncthing add guiAddress parameter #48119

Merged
merged 2 commits into from Oct 14, 2018
Merged

nixos/modules: services.syncthing add guiAddress parameter #48119

merged 2 commits into from Oct 14, 2018

Conversation

mrVanDalo
Copy link
Contributor

Motivation for this change

To make it possible to configure your syncthing from a remote machine.

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

infinisil
infinisil requested changes Oct 13, 2018
View reviewed changes
nixos/modules/services/networking/syncthing.nix Outdated
@@ -16,6 +16,14 @@ in {
available on http://127.0.0.1:8384/.
'';

guiAddress = mkOption {
type = types.string;
Copy link
Member

Choose a reason for hiding this comment

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

Use types.str instead

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 used types.string for consistency ( it is used in the other options as well )

Copy link
Member

Choose a reason for hiding this comment

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

Change all of them to str then :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

nixos/modules/services/networking/syncthing.nix Outdated
type = types.string;
default = "127.0.0.1:8384";
description = ''
Override GUI listen address.
Copy link
Member

Choose a reason for hiding this comment

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

How about "Address to server the GUI on" instead? The fact that it overrides the default shouldn't be in the options description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

peterhoeg
peterhoeg requested changes Oct 14, 2018
View reviewed changes
Copy link
Member

@peterhoeg peterhoeg left a comment

Choose a reason for hiding this comment

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

Looks good once @infinisil 's concerns have been addressed.

@peterhoeg peterhoeg merged commit abe0e22 into NixOS:master Oct 14, 2018
@peterhoeg
Copy link
Member

Looks good, thank you for your contribution @mrVanDalo

@colemickens colemickens mentioned this pull request Nov 5, 2018
Closed
@matthewbauer
Copy link
Member

matthewbauer commented Nov 5, 2018 •
edited

Could you add a release note that explains the guiAddress change? This appears to override the default from configDir.

#49805

@mrVanDalo
Copy link
Contributor Author

@matthewbauer I don't know how to write a release note.

@peterhoeg
Copy link
Member

peterhoeg commented Nov 7, 2018 •
edited

All, please note that the latest version .52 is just out which supports a "unix://" parameter to guiAddress which would be neat to support.

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

@infinisil infinisil infinisil requested changes

@peterhoeg peterhoeg peterhoeg approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mrVanDalo @peterhoeg @matthewbauer @infinisil @GrahamcOfBorg