Add step_certificate role, supporting multiple provisioners
#129
Conversation
|
I'm not sure the tests will pass yet. I don't really expect them to pass, but even if they do, it's only because I copied the |
There was a problem hiding this comment.
That looks good to me! Having a separate tasks file for each type of provisioner seems like a good approach to me, and the rest of the role is clearly structured as well.
The only thing that I'd like to see addressed is the logic that determines which tasks files to include for each provisioner (in tasks/cert.yml) - see my comment about that. The current version seems overly complex for what should be a relatively simple task. That said, i might be missing something, so please let me know if I am!
Aside from that, the tests obviously need to be updated - I suggest creating a separate molecule scenario for each supported provisioner type.
Overall though, I'm really happy with this! I'd love to merge it once the issues mentioned above have been ironed out.
| driver: | ||
| name: docker | ||
| network: | ||
| - name: smallstep_acme_cert |
There was a problem hiding this comment.
Should probably be changed to reflect the role name
There was a problem hiding this comment.
yeah - no testing implemnted yet.
eengstrom
force-pushed
the
add-step_certificate-role
branch
from
b517f16 to
34fac99
Compare
|
Looks good! I'd be happy to merge this once the tests have been adjusted to cover both provisioners 👍 |
This role can accommodate all different types of provisioners. For the currently implementation, only: - ACME - JWK If the requested provisioner is not supported, role will fail with appropriate message. If this role is adopted, suggest we deprecate `step_acme_cert`. FIXES: maxhoesel-ansible#127
eengstrom
force-pushed
the
add-step_certificate-role
branch
from
34fac99 to
38e0825
Compare
|
@maxhoesel and @eengstrom are you guys still looking to add this? I think this would be hugely helpful to support JWK provisioners. Thanks. |
|
@salekseev -- Yes, at least I am still interested. I'm still using a fork I have, but it's sorely out of date with respect @maxhoesel 's mainline code base, but have need to rebase anyway. I'm open to more help to develop the testing that Max (rightly) wants, if you have inclination. |
|
I would also still be happy to merge this, and I might be able to help out with the tests as well. Unfortunately I haven't had a lot of time to work on this collection recently, and that's unlikely to change for about another month or so. I'd be happy to have a go at this once things calm down for me though! |
Adding
step_certificateas a new role that can accommodate all different types of provisioners.For the current implementation, only supports:
If the requested provisioner is not supported, the role will fail with appropriate message.
If this role is adopted, I suggest we deprecate
step_acme_cert, and will resolve #127.NOTE - this PR is a DRAFT, and I'm soliciting feedback.