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

nixos/captive-browser: make it work without a fixed interface #99355

Merged
merged 1 commit into from Feb 12, 2021
Merged

nixos/captive-browser: make it work without a fixed interface #99355

merged 1 commit into from Feb 12, 2021

Conversation

peterhoeg
Copy link
Member

@peterhoeg peterhoeg commented Oct 2, 2020 •
edited

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.

@Ma27 Ma27 requested review from Ma27 and volth October 2, 2020 09:19
@flokli
Copy link
Contributor

flokli commented Oct 18, 2020

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

@peterhoeg
Copy link
Member Author

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
nixos/captive-browser: make it work without a fixed interface
d852c56 
nixpkgs-fmt makes this a lot noisier than it has to be
@peterhoeg
Copy link
Member Author

This works fine here @flokli. All volth's comments are gone, so I don't remember if all have been addressed. Do you have any additional comments?

@peterhoeg peterhoeg merged commit 04e9e9e into NixOS:master Feb 12, 2021
@peterhoeg peterhoeg deleted the f/captive_browser branch February 12, 2021 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Awaiting requested review from Ma27

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@peterhoeg @flokli