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

oauth2_proxy: create new module for service #15283

Merged
merged 1 commit into from
Jun 9, 2016

Conversation

jml
Copy link
Contributor

@jml jml commented May 7, 2016

This patch adds a module for oauth2_proxy, exposing all of the options for the binary in what I hope is a sensible manner.

I've tested the module on my own (alas, private) NixOS deployment. I looked through the todo list below and they all seem like options relevant to packages rather than modules.

This is my first substantive patch to NixOS, so please let me know if I'm missing any conventions.

Thanks,
jml

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
    • OS X
    • Linux
  • 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 change is Reviewable

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers

serviceConfig = {
User = "oauth2_proxy";
Restart = "always";
# Arbitrarily chosen value.
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's arbitrary, maybe rely on the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Removed.

default = null;
description = ''
Authentication endpoint.

Copy link
Member

Choose a reason for hiding this comment

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

Note that empty lines will not show up in the manual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I think the newlines here make it easier to read in the file. Any objection to me leaving them as is?

@jml
Copy link
Contributor Author

jml commented May 23, 2016

Thank you for the comments & sorry for the delay in replying. I think I've addressed everything that has been raised, so please take a look.

@joachifm
Copy link
Contributor

According to travis there's a problem with one of the option declarations

@jml
Copy link
Contributor Author

jml commented Jun 1, 2016

Option declaration fixed. AFAICT the remaining build failure isn't one of mine.

@joachifm
Copy link
Contributor

joachifm commented Jun 1, 2016

Please squash when you're ready to merge.

@jml
Copy link
Contributor Author

jml commented Jun 2, 2016

Done.

--cookie-expire=${cfg.cookie.expire} \
--cookie-httponly=${fromBool cfg.cookie.httpOnly} \
--cookie-name=${cfg.cookie.name} \
--cookie-secret=${cfg.cookie.secret} \
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, first: nice work.

I just tried this and I think the values need either explicit escaping or at least '..' around the arguments

[1]
https://www.freedesktop.org/software/systemd/man/systemd-escape.html

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.

I've rebased as well to avoid an extra roundtrip.

@teh
Copy link
Contributor

teh commented Jun 9, 2016

@joachifm - I tested this and it works as advertised so I think we can merge.

@joachifm joachifm merged commit e2e2840 into NixOS:master Jun 9, 2016
@joachifm
Copy link
Contributor

joachifm commented Jun 9, 2016

@tel cool, thank you for additional testing. @jml thank you for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants