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

Add certbot-dns-ovh plugin support #195

Open
wants to merge 91 commits into
base: master
Choose a base branch
from
Open

Conversation

aellert
Copy link

@aellert aellert commented Aug 8, 2019

Pull Request (PR) description

Add dns-ovh support based on dns_rfc2136 implementation

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Overall nice. I'm starting to think we need some abstraction ($plugin_package_prefix and a letsencrypt::plugin` define) but it's probably fine to wait for another plugin to see which abstraction we need.

$plugin_args = [
"--cert-name '${title}' -d",
"'${_domains}'",
"--dns-ovh-credentials ${letsencrypt::plugin::dns_ovh::config_dir}/dns-ovh.ini",
Copy link
Member

Choose a reason for hiding this comment

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

Should the plugin declare a $config_file variable? Can be inside the body of the class. That way you don't rely on these two matching but can statically check it.

Copy link
Author

Choose a reason for hiding this comment

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

I don't know exactly what you mean. Can you give me more details ?

Copy link
Member

Choose a reason for hiding this comment

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

I meant something like (very shortened):

class letsencrypt::plugin::dns_ovh {
  $config_file = "${letsencrypt::plugin::dns_ovh::config_dir}/dns-ovh.ini"

  file { $config_file:
    # ...
  }
}

Then you can use it here:

"--dns-ovh-credentials ${letsencrypt::plugin::dns_ovh::config_file}",

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I changed $config_dir by $config_file in this PR

@@ -0,0 +1,59 @@
# == Class: letsencrypt::plugin::dns_ovh
Copy link
Member

Choose a reason for hiding this comment

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

Could you format this using puppet-strings style?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's done here
Should I do another PR ?

Copy link
Member

Choose a reason for hiding this comment

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

It's fine here. A separate PR to convert the rest of the module would be appreciated though :)

Copy link
Author

Choose a reason for hiding this comment

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

Ok, next week I will probably have some time to do it !

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

It looks correct. Could you have a look at adding tests to this as well? Other than that 👍 to merging.

@aellert
Copy link
Author

aellert commented Aug 8, 2019

It looks correct. Could you have a look at adding tests to this as well? Other than that 👍 to merging.

I never write tests before but I'm gonna try.

@ekohl
Copy link
Member

ekohl commented Aug 19, 2019

Any luck so far? I'd happy to make suggestions when you're stuck.

@aellert
Copy link
Author

aellert commented Aug 22, 2019

Hi Ewoud,
I was busy last days..
But I managed to add tests (largely inspired by rfc2136 plugin) and update README.
Note that travis failed because package python3-certbot-dns-ovh is only available from Debian 10.
How can we handle this ?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks good, though I'd like to avoid specific distribution checks in the code if possible.

#
# @see https://certbot-dns-ovh.readthedocs.io
#
# === Parameters:
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer needed with puppet-strings.

Suggested change
# === Parameters:


Note:

For Debian based OS, this plugin is compatible from Debian 10
Copy link
Member

Choose a reason for hiding this comment

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

Why is it not supported? If there are no packages, would it be better to set the package name to undef on those and add a check in the class that if manage_package is true, the package must be set?

Dan33l and others added 13 commits August 28, 2019 09:34
…encies_versions

Raise upper bound version of stdlib & vcsrepo
package_name is a mandatory parameter. This means that the default value
cannot be `undef`.
Previously, we had no hiera entry for this key for Debian 9 / Ubuntu
16.04. Instead we had it multiple times, for Debian 10 and Ubuntu 18.04.
We can reduce it to a single occurance within the Debian.yaml for the
whole os family.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.