search

luisico / cert-manager-webhook-infoblox-wapi

InfoBlox WAPI webhook for cert-manager
Apache License 2.0
4 stars 8 forks source link

Add possibility to configure cert duration #5

Closed ONordander closed 2 years ago

ONordander commented 2 years ago

Hey. This PR adds the possibility to configure the duration of the certificates which we need for our use-case.

Not sure how to bump the versions, do the app version follow the chart version? If it's incorrect just tell me how you version it and I can update.

This should be a non-breaking change as I added the previous values as default.

ONordander commented 2 years ago

@luisico could you look at this?

luisico commented 2 years ago

Hi @ONordander, thanks for this PR, and sorry for the late reply.

The idea sound good to me, I do have a few comments, though:

  1. Add some documentation, probably under section Install infoblox-wapi webhook.
  2. I think using the webhook prefix in the parameters is a bit confusing. webhook is defined in _helpers.tpl as helm helper functions. I suggest to rename the parameters in values.yaml like:
rootCACertificate:
  duration: "43800h"
servingCertificate:
  duration: "8760h"

And change their interpolation appropriately.

Other than that, looks good to me, although I haven't been able to test, and won't be able for a while (moving a couple of oceans apart at the moment!).

ONordander commented 2 years ago

Hi @ONordander, thanks for this PR, and sorry for the late reply.

The idea sound good to me, I do have a few comments, though:

1. Add some documentation, probably under section [Install infoblox-wapi webhook](https://github.com/luisico/cert-manager-webhook-infoblox-wapi#install-infoblox-wapi-webhook).

2. I think using the `webhook` prefix in the parameters is a bit confusing. `webhook` is defined in `_helpers.tpl` as helm helper functions. I suggest to rename the parameters in `values.yaml` like:
rootCACertificate:
  duration: "43800h"
servingCertificate:
  duration: "8760h"

And change their interpolation appropriately.

Other than that, looks good to me, although I haven't been able to test, and won't be able for a while (moving a couple of oceans apart at the moment!).

  1. Done, added a section for all the values as that made more sense to me.
  2. Agree, done :)

I tested it locally with helm template -f values.yaml -n cert-manager .

luisico commented 2 years ago

Thanks @ONordander, LGTM, will merge

luisico commented 2 years ago

@ONordander Released as 1.5.2. Let me know if you have any problems. Thanks for your help. Luis

ONordander commented 2 years ago

@ONordander Released as 1.5.2. Let me know if you have any problems. Thanks for your help. Luis

Worked very well, thanks :)