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/captive-browser: make it work without a fixed interface #99355

Open
wants to merge 1 commit into
base: master
from

Conversation

@peterhoeg
Copy link
Member

@peterhoeg peterhoeg commented Oct 2, 2020

Motivation for this change

Currently, the module requires us to specify an interface but that technically isn't necessary as we could simply read DNS for all interfaces.

This PR makes things work without specifying an interface.

Commits will be squashed on merge.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
else if config.networking.dhcpcd.enable then
"${pkgs.dhcpcd}/bin/dhcpcd -U ${escapeShellArg cfg.interface} | ${pkgs.gnugrep}/bin/fgrep domain_name_servers"
"${pkgs.dhcpcd}/bin/dhcpcd ${iface "-U"} | ${pkgs.gnugrep}/bin/fgrep domain_name_servers"

This comment has been minimized.

@volth

volth Oct 2, 2020
Contributor

it will produce '-U eth0' quoting them both

actually, I would inline the iface function to make it more readable

@volth
Copy link
Contributor

@volth volth commented Oct 2, 2020

Also making cfg.interface nullable looks better, because after the change if bindInterface is false, cfg.interface not used at all

@flokli
Copy link
Contributor

@flokli flokli commented Oct 18, 2020

@peterhoeg I very much like this. Can you address the changes requested by @volth?

nixpkgs-fmt makes this a lot noisier than it has to be
@peterhoeg peterhoeg force-pushed the peterhoeg:f/captive_browser branch from e7be374 to 3919073 Oct 20, 2020
@peterhoeg
Copy link
Member Author

@peterhoeg peterhoeg commented Oct 20, 2020

Good catch of the quoting @volth - we now quote individually. We could definitely inline the iface stuff but it just looks a lot neater (IMHO) without. You feel strongly about this? Another option is additional comments of course.

nixpkgs-fmt has made this PR a lot noisier than it otherwise should be so here is a link to it with white space changes ignored: https://github.com/NixOS/nixpkgs/pull/99355/files?w=1

I'll try this out tomorrow morning where I know I'll be exposed to a portal.

@peterhoeg
Copy link
Member Author

@peterhoeg peterhoeg commented Oct 20, 2020

So consider this TIP for now.

@volth
Copy link
Contributor

@volth volth commented Oct 20, 2020

Escaping is wrong again (it might produce dev show '' eth0, quoting empty string)
That is why I am pro-inlining: this "neatness" hides mistakes very well

Also

- concatStringsSep " " (map escapeShellArg
+ escapeShellArgs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.