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/xl2tpd: add the xl2tpOptions #30677

Closed
wants to merge 1 commit into from
Closed

nixos/xl2tpd: add the xl2tpOptions #30677

wants to merge 1 commit into from

Conversation

jpotier
Copy link
Contributor

@jpotier jpotier commented Oct 22, 2017

This option allows the user to give a full configuration file, and
bypass the — sometimes problematic — default configuration.

Motivation for this change

The module is not usable as-is with my company's setup. I suspect this will be the case for other people, so the simplest option is to give users the choice to provide a full config file.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
  • Fits CONTRIBUTING.md.

This option allows the user to give a full configuration file, and
bypass the — sometimes problematic — default configuration.
@jpotier
Copy link
Contributor Author

jpotier commented Oct 22, 2017

@obadz you are the original writter of the module, any thoughts?

@jpotier
Copy link
Contributor Author

jpotier commented Oct 22, 2017

@abbradar you are the maintainer of the underlying xl2tpd package, I wish to bring this to your attention, if you are interested.

@obadz
Copy link
Contributor

obadz commented Oct 23, 2017

@jpotier, is there really no way for you to add the options you care about to the module rather than provide a string-based fallback?

@jpotier
Copy link
Contributor Author

jpotier commented Oct 23, 2017

Hmm, sure, there should be a way. This will require much more work though. I'll try and come up with something more detailed, and keeping your options.

I proposed this, because I've seen it implemented in other modules this way.

In any case, I'll have to eliminate some of the defaults.

@obadz
Copy link
Contributor

obadz commented Oct 23, 2017

Is this related to your E-mail where you try to set this up as a client rather than server?

@jpotier
Copy link
Contributor Author

jpotier commented Oct 23, 2017

yep, it is.

@obadz
Copy link
Contributor

obadz commented Oct 23, 2017

Do you want to try the networkmanager module for client side instead?

@jpotier
Copy link
Contributor Author

jpotier commented Oct 23, 2017 via email

@obadz
Copy link
Contributor

obadz commented Oct 23, 2017

Yes networkmanager lives outside of nix. If you want to avoid that you'll need to do this from a module indeed.

This module was meant to configure the server side. Not sure whether client version should be another module or same module with different options. My first guess would be separate module but feel free to propose what you feel is best.

@jpotier
Copy link
Contributor Author

jpotier commented Oct 23, 2017 via email

@jpotier
Copy link
Contributor Author

jpotier commented Oct 23, 2017

Well, now that I thought about it a little bit more, I don't get why we should separate client and server. I'll just enable more options, but people can choose by setting the options.

A good next point on the roadmap would be to start several instances of the service.

@matthewbauer
Copy link
Member

Status of this?

@jpotier
Copy link
Contributor Author

jpotier commented Apr 22, 2018

Let's close this one. I'll PR something else when I get the time.

@jpotier jpotier closed this Apr 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants