Fix #1617 Add dh_params resource #1618
Conversation
Signed-off-by: Doc Walker <[email protected]>
4-20ma
force-pushed
the
1617-dh-params-resource
branch
from
b662331 to
841c178
Compare
|
Oh super nice @4-20ma thank you!! |
|
@4-20ma this totally rocks. I'll be reviewing this later today and likely get it merged! Nicely done! |
| @@ -19,6 +19,7 @@ | |||
| include_recipe('os_prepare::iis_site') | |||
| include_recipe('os_prepare::iptables') unless node['osprepare']['docker'] | |||
| include_recipe('os_prepare::x509') | |||
| include_recipe('os_prepare::dh_params') | |||
There was a problem hiding this comment.
Normally I'd add this line in alphabetical order, but the existing recipes are already out of order. Let me know if this requires adjustment.
There was a problem hiding this comment.
it doesn't matter - this is fine :)
| @@ -138,6 +138,8 @@ def md.directory? | |||
| 'test_certificate.rsa.crt.pem' => mockfile.call('test_certificate.rsa.crt.pem'), | |||
| 'test_certificate.rsa.key.pem' => mockfile.call('test_certificate.rsa.key.pem'), | |||
| 'test_ca_public.key.pem' => mockfile.call('test_ca_public.key.pem'), | |||
| # Test DH parameters, 2048 bit long safe prime, generator 2 for dh_params in PEM format | |||
| 'dh_params.dh_pem' => mockfile.call('dh_params.dh_pem'), | |||
There was a problem hiding this comment.
Normally I'd add this line in alphabetical order, but the existing recipes are already out of order. Let me know if this requires adjustment.
There was a problem hiding this comment.
it doesn't matter - this is fine :) We should totally alphabetize these though for ease.
|
@chris-rock, @arlimus, @adamleff thanks for the support/consideration. Let me know if there are any tweaks you'd like to see. I wasn't sure what order to include some of the tests/helpers. Refer to my comments in the Files Changed tab. |
There was a problem hiding this comment.
@4-20ma this is super, nicely done.
I just had a comment on the p and g method aliases you created. While they make sense if you're familiar with DH parameters, since we don't document them, I'm inclined to ask they be removed. What are your thoughts?
lib/resources/dh_params.rb
Outdated
| end | ||
|
|
||
| # its('g') { should eq 2 } | ||
| alias g generator |
There was a problem hiding this comment.
I'm inclined to suggest removing these since they're not documented and we don't have a pattern of including aliases in other resources that I'm aware of.
There was a problem hiding this comment.
We try to avoid those in most cases. In this case it makes sense to have them, but if we keep them, we should document those.
There was a problem hiding this comment.
I have no issue removing them; simplicity is a good thing.
lib/resources/dh_params.rb
Outdated
| end | ||
|
|
||
| # its('p') { should eq '00:91:a0:15:89:e5:bc:38:93:12:02:fc:...' } | ||
| alias p modulus |
There was a problem hiding this comment.
I'm inclined to suggest removing these since they're not documented and we don't have a pattern of including aliases in other resources that I'm aware of.
There was a problem hiding this comment.
What is the project preference for this change? Should I rebase and do a forced push? Or simply push a new commit?
There was a problem hiding this comment.
I don't think we have an official one. 🙂 I'd ask that you just push up a new commit. If there's more than 2 or 3 commits, I'll squash before merge on your behalf.
There was a problem hiding this comment.
It just makes reviewing changes between review exchanges SO much easier!
Signed-off-by: Doc Walker <[email protected]>
Signed-off-by: Doc Walker <[email protected]>
|
Again, @4-20ma... thank you for the great contribution to InSpec! |
Signed-off-by: Doc Walker [email protected]