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

Refactor openssl_csr module, add openssl_csr_pipe module #123

Merged
merged 13 commits into from
Oct 27, 2020

Conversation

felixfontein
Copy link
Contributor

SUMMARY

(Almost) same change to openssl_csr as done for #119. The main exception is that the result is not an action module, but a "real" module. This allows to run the modue also on remove machines.

Implements the openssl_csr part of ansible/ansible#63553.

(CC @onitake @dnmvisser who created/liked that issue.)

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

openssl_csr
openssl_csr_pipe

@onitake
Copy link

onitake commented Oct 12, 2020

My original request was focused on the use case of a dedicated CA machine that's responsible for signing certificates, so having a proper module is good, and this doesn't exclude localhost usage.

I'm a bit confused about naming though: You're replacing openssl_csr with openssl_csr_pipe? What's the point of a new name when you remove the old module?
I'm not sure a separate module makes sense if there is enough overlap between the two.

@felixfontein
Copy link
Contributor Author

My original request was focused on the use case of a dedicated CA machine that's responsible for signing certificates, so having a proper module is good, and this doesn't exclude localhost usage.

To fully do this, the same refactoring/addition of new module needs also to be done for x509_certificate. Then this will be possible without having to write something to disk on the CA machine (this PR only saves you writing the CSR to disk somewhere).

I'm a bit confused about naming though: You're replacing openssl_csr with openssl_csr_pipe? What's the point of a new name when you remove the old module?

The openssl_csr module is still there with the same functionality as before. The only difference is that there's now another module, openssl_csr_pipe, with very similar functionality, except that it doesn't operate on the CSR file.

I'm not sure a separate module makes sense if there is enough overlap between the two.

IMO it is a lot cleaner if all the file-specific options and return values aren't there when you don't need them.

@onitake
Copy link

onitake commented Oct 12, 2020

Oh, I see. I got confused by this part:
image

That looked like you removed the file completely, but I guess it only appeared because you moved the documentation to doc_fragments.

@felixfontein
Copy link
Contributor Author

I also moved a lot of its code to module_utils, so its size reduced drastically. But it's still there, and due to the doc fragment and module_utils it still does the same as before. (Or should do, modulo bugs :) )

@felixfontein felixfontein mentioned this pull request Oct 14, 2020
10 tasks
@felixfontein
Copy link
Contributor Author

ready_for_review

Copy link
Contributor

@MarkusTeufelberger MarkusTeufelberger left a comment

Choose a reason for hiding this comment

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

Similar to the openssl_privatekey_pipe one, I mostly focused on docs and some smaller things, all in all it looks great! :-)

plugins/doc_fragments/module_csr.py Outdated Show resolved Hide resolved
plugins/doc_fragments/module_csr.py Outdated Show resolved Hide resolved
plugins/modules/openssl_csr.py Outdated Show resolved Hide resolved
plugins/modules/openssl_csr_pipe.py Show resolved Hide resolved
plugins/modules/openssl_csr_pipe.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Contributor Author

The last commit is needed because of ansible/ansible#72334.

@felixfontein
Copy link
Contributor Author

@MarkusTeufelberger is the helper in 86758a2 fine for you? Is there anything else you would like to be improved or changed?

@MarkusTeufelberger
Copy link
Contributor

No, I love it. Thanks for all the work!

/shipit

@felixfontein felixfontein merged commit 9792188 into ansible-collections:main Oct 27, 2020
@felixfontein felixfontein deleted the openssl_csr_pipe branch October 27, 2020 11:37
@felixfontein
Copy link
Contributor Author

@MarkusTeufelberger thanks a lot for reviewing this! :)

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.

3 participants