-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add DH param generator option #913
Add DH param generator option #913
Conversation
@jwilder would it be possible to re-run tests? The failed tests was triggered when it was trying to generate dh param which could take time. I have run tests on my computer and it passed every single test cases |
This makes sense to me, as the contributor that wrote the DHPARAM generation code. One suggestion I would make is to rename |
@kamermans Yes I do agree to, it's always a good idea to be consistence. Will make that change in the future commit |
Is there a reason this was never merged? I'm also using letsencrypt-nginx-proxy-companion and this adds an unnecessary delay. |
Not quite sure if this is the right place to ask this, but is the |
@iamdann yeah, looks like there's a small user experience bug here. If you provide
I think the better user experience would be to set both variables as default if they aren't set, not rely on positional arguments: diff --git a/generate-dhparam.sh b/generate-dhparam.sh
index 4099dde..aed055e 100755
--- a/generate-dhparam.sh
+++ b/generate-dhparam.sh
@@ -4,2 +4,2 @@
-DHPARAM_BITS=${1:-2048}
-GENERATE_DHPARAM=${2:-true}
+DHPARAM_BITS=${DHPARAM_BITS:-2048}
+GENERATE_DHPARAM=${DHPARAM_GENERATION:-true I'm also not sure why there's both @panteparak @kamermans if you agree I'd be happy to open a PR. |
That all makes sense to me, and a PR would be appreciated! You're welcome to fix the variable naming to make it consistent as well :) |
!913 added `DHPARAM_GENERATION` as a positional argument to generate-dhparam.sh. However, since it was the second positional argument, `DHPARAM_BITS` would also have to be defined or `DHPARAM_GENERATION` would be read into `DHPARAM_BITS`. This changes the arguments to be inherited variables which do not depend on order, just declaration. Also change instances of `GENERATE_DHPARAM` to `DHPARAM_GENERATION` since it's unnecessary to have another variable. I think `GENERATE_DHPARAM` is actually a better name (verb vs. noun), but `DHPARAM_GENERATION` is already defined and may break someone if changed. Addresses nginx-proxy#913 (comment)
PR nginx-proxy#913 added `DHPARAM_GENERATION` as a positional argument to generate-dhparam.sh. However, since it was the second positional argument, `DHPARAM_BITS` would also have to be defined or `DHPARAM_GENERATION` would be read into `DHPARAM_BITS`. This changes the arguments to be inherited variables which do not depend on order, just declaration. Also change instances of `GENERATE_DHPARAM` to `DHPARAM_GENERATION` since it's unnecessary to have another variable. I think `GENERATE_DHPARAM` is actually a better name (verb vs. noun), but `DHPARAM_GENERATION` is already defined and may break someone if changed. Addresses nginx-proxy#913 (comment)
PR nginx-proxy#913 added `DHPARAM_GENERATION` as a positional argument to generate-dhparam.sh. However, since it was the second positional argument, `DHPARAM_BITS` would also have to be defined or `DHPARAM_GENERATION` would be read into `DHPARAM_BITS`. This changes the arguments to be inherited variables which do not depend on order, just declaration. Also change instances of `GENERATE_DHPARAM` to `DHPARAM_GENERATION` since it's unnecessary to have another variable. I think `GENERATE_DHPARAM` is actually a better name (verb vs. noun), but `DHPARAM_GENERATION` is already defined and may break someone if changed. Addresses nginx-proxy#913#issuecomment-476014691
@kamermans I opened #1263 ICYMI. |
PR nginx-proxy#913 added `DHPARAM_GENERATION` as a positional argument to generate-dhparam.sh. However, since it was the second positional argument, `DHPARAM_BITS` would also have to be defined or `DHPARAM_GENERATION` would be read into `DHPARAM_BITS`. This changes the arguments to be inherited variables which do not depend on order, just declaration. Also change instances of `GENERATE_DHPARAM` to `DHPARAM_GENERATION` since it's unnecessary to have another variable. I think `GENERATE_DHPARAM` is actually a better name (verb vs. noun), but `DHPARAM_GENERATION` is already defined and may break someone if changed. Addresses nginx-proxy#913 (comment)
With the integration of letsencrypt-nginx-proxy-companion. When both containers start at the same time, sometimes both containers are trying to generate Dh-Param at the same time which could be heavily taxing thus, adding an option to disable the generation of dh param and only let
letsencrypt-nginx-proxy-companion
do the generating, or offers an already generated dh param.