Add support for the Certbot Gandi plugin #295

cible · 2022-08-26T09:42:32Z

Pull Request (PR) description

Adding feature support for the certbot-plugin-gandi created by @obynio

The plugin use the Production API key.

This Pull Request (PR) fixes the following issues

N/A

… available

smortex · 2022-08-29T18:08:33Z

data/FreeBSD-family.yaml

@@ -5,3 +5,4 @@ letsencrypt::cron_owner_group: 'wheel'
 letsencrypt::plugin::dns_rfc2136::package_name: 'py38-certbot-dns-rfc2136'
 letsencrypt::plugin::dns_route53::package_name: 'py38-certbot-dns-route53'
 letsencrypt::plugin::dns_cloudflare::package_name: 'py38-certbot-dns-cloudflare'
+letsencrypt::plugin::dns_gandi::package_name: 'certbot-plugin-gandi'


There are currently no FreeBSD port for this :-/

The name would probably be py38-certbot-dns-gandi if there was, and in fact py39-certbot-dns-gandi because the default version of Python changed recently on FreeBSD.

I opened #296 to adjust the FreeBSD version.

Do you think it's a good idea to install the package with pip? I'm not sure on how to install them on freebsd and centos. I will ask help to a colleague

As a FreeBSD user, I would prefer to use a package rather installing using pip. Anybody can submit ports to FreeBSD, and they are ultimately available as packages. My guess is that if the port does not exist, nobody needed it so much they did a port for it, and maybe it is not worth it?

I would personally not set a package_name for FreeBSD in Hiera and make $letsencrypt::plugin::dns_gandi::package_name mandatory so that it fails hard if someone attempt to use it on FreeBSD.

ekohl · 2022-08-31T09:43:40Z

data/os/Debian/11.yaml

@@ -0,0 +1,3 @@
+---
+letsencrypt::plugin::dns_gandi::package_provider: apt


Why do you need to set the package provider? That's rarely needed.

You're right, I will remove it

ekohl · 2022-08-31T09:44:07Z

manifests/plugin/dns_gandi.pp

+    if $package_provider {
+      package { $package_name:
+        ensure   => installed,
+        provider => $package_provider,
+      }
+    }
+    else {
+      package { $package_name:
+        ensure => installed,
+      }
+    }


You can just pass undef along:

Suggested change

if $package_provider {

package { $package_name:

ensure => installed,

provider => $package_provider,

}

}

else {

package { $package_name:

ensure => installed,

}

}

package { $package_name:

ensure => installed,

provider => $package_provider,

}

ekohl · 2022-08-31T09:45:44Z

manifests/plugin/dns_gandi.pp

+  if $api_key {
+    $ini_vars = {
+      'certbot_plugin_gandi:dns_api_key' => $api_key,
+    }
+  } else {
+    fail('api_key not provided for certbot dns gandi plugin.')
+  }


The data type guarantees it's set

Suggested change

if $api_key {

$ini_vars = {

'certbot_plugin_gandi:dns_api_key' => $api_key,

}

} else {

fail('api_key not provided for certbot dns gandi plugin.')

}

$ini_vars = {

'certbot_plugin_gandi:dns_api_key' => $api_key,

}

ekohl · 2022-08-31T09:47:39Z

data/RedHat-family.yaml

@@ -3,3 +3,4 @@ letsencrypt::configure_epel: true
 letsencrypt::plugin::dns_rfc2136::package_name: 'python3-certbot-dns-rfc2136'
 letsencrypt::plugin::dns_route53::package_name: 'python3-certbot-dns-route53'
 letsencrypt::plugin::dns_cloudflare::package_name: 'python3-certbot-dns-cloudflare'
+letsencrypt::plugin::dns_gandi::package_name: 'certbot-plugin-gandi'


This doesn't look correct.

treydock · 2022-11-16T18:34:26Z

@cible Is this ready for review? The pull request is currently marked as a draft.

manifests/plugin/dns_gandi.pp

ekohl · 2022-11-16T20:43:34Z

manifests/plugin/dns_gandi.pp

+class letsencrypt::plugin::dns_gandi (
+  String[1]            $api_key,
+  String[1]  $package_name,
+  Optional[String[1]]  $package_provider = undef,


This is an unused parameter. Shouldn't it be passed to the package resource?

manifests/plugin/dns_gandi.pp

spec/spec_helper_acceptance.rb

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

ekohl · 2022-11-21T15:09:07Z

spec/classes/plugin/dns_gandi_spec.rb

+
+          it do
+            if package_name.nil?
+              is_expected.not_to compile


A better way is to test for the error:

Suggested change

is_expected.not_to compile

is_expected.to compile.and_raise_error(/A Matcher For The Error/)

ekohl · 2022-11-21T15:09:46Z

spec/defines/letsencrypt_certonly_spec.rb

+          PUPPET
+        end
+
+        it { is_expected.not_to compile.with_all_deps }


Here too it's better to compile and test for a specific error

For this one, the module won't work with an empty api_key and I don't know how to handle the error:

13) letsencrypt::certonly on fedora-36-x86_64 based operating systems with dns-gandi plugin without api_key is expected to fail to compile and raise an error matching /\/expects a value for parameter 'api_key'\// Failure/Error: it { is_expected.to compile.and_raise_error(%r{/expects a value for parameter 'api_key'/}) } error during compilation: Evaluation Error: Error while evaluating a Resource Statement, Class[Letsencrypt::Plugin::Dns_gandi]: expects a value for parameter 'api_key' (line: 6, column: 11

it { is_expected.to compile.and_raise_error(%r{/expects a value for parameter 'api_key'/}) } is not correct?

The / is redundant. In Ruby you can define a regex as /my regex/ or %r{my regex} but %r{/my regex/} means you expect / in the output. This is useful if you want to test /var/lib is empty is in the error. Compare %r{/var/lib is empty} to /\/var\/lib is empty/.

manifests/plugin/dns_gandi.pp

spec/classes/plugin/dns_gandi_spec.rb

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

cible · 2022-11-23T14:58:27Z

It seem ok for me. Sorry for being so slow and bad with ruby. Did I have to do something else? rebase?

bastelfreak · 2023-01-15T09:51:54Z

@cible yes, this needs a rebase. I think we can merge if afterwards 👍

cible added 2 commits August 26, 2022 10:07

Adding feature support for the Certbot plugin Gandi

de30daa

Use pip to install certbot-plugin-gandi by default but use package if…

bf81b49

… available

smortex reviewed Aug 29, 2022

View reviewed changes

smortex and others added 3 commits August 30, 2022 15:43

Merge voxpupuli#296

863985a

Make the provideroptional

471c64e

Merge branch 'master' into feature/plugins/dns-gandi

37200dc

ekohl reviewed Aug 31, 2022

View reviewed changes

cible added 4 commits August 31, 2022 13:20

Remove package_name if non existent

f062f57

Merge branch 'master' into feature/plugins/dns-gandi

2d89536

Only test on debian-11 & ubuntu 20.04

b51de96

Add function to check if platform is supported by gandi plugin

d62ea76

treydock requested review from ekohl and smortex November 16, 2022 18:33

treydock added the enhancement New feature or request label Nov 16, 2022

ekohl reviewed Nov 16, 2022

View reviewed changes

Apply suggestions from code review

09ba2f7

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

ekohl reviewed Nov 21, 2022

View reviewed changes

cible added 6 commits November 22, 2022 10:37

Add support for ubuntu 22.04

e010968

Remove now useless $package_provider

9323fb2

Merge branch 'master' into feature/plugins/dns-gandi

0287c8b

Better tests for handling unsupported platforms

b2fe5ce

Syntax fix

6c82c1a

Fix variable name

ed0cfca

ekohl reviewed Nov 22, 2022

View reviewed changes

manifests/plugin/dns_gandi.pp Outdated Show resolved Hide resolved

spec/classes/plugin/dns_gandi_spec.rb Outdated Show resolved Hide resolved

spec/classes/plugin/dns_gandi_spec.rb Outdated Show resolved Hide resolved

spec/classes/plugin/dns_gandi_spec.rb Outdated Show resolved Hide resolved

cible and others added 5 commits November 22, 2022 13:19

Update spec/classes/plugin/dns_gandi_spec.rb

f8457b7

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>

Fix alignment

17c1b61

Remove now useless code

47824f6

Fix regexps

ef0d4f5

Fix tests

a513b57

Fix indent

1e65a10

cible marked this pull request as ready for review November 23, 2022 14:58

Merge branch 'master' into feature/plugins/dns-gandi

3996f08

kenyon added merge-conflicts needs-rebase needs-squash labels Dec 3, 2023

kenyon changed the title ~~Adding feature support for the Certbot plugin Gandi~~ Add support for the Certbot Gandi plugin Dec 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the Certbot Gandi plugin #295

Add support for the Certbot Gandi plugin #295

cible commented Aug 26, 2022

smortex Aug 29, 2022

smortex Aug 29, 2022

cible Aug 30, 2022

smortex Aug 31, 2022

ekohl Aug 31, 2022

cible Nov 21, 2022

ekohl Aug 31, 2022

ekohl Aug 31, 2022

ekohl Aug 31, 2022

treydock commented Nov 16, 2022

ekohl Nov 16, 2022

ekohl Nov 21, 2022

ekohl Nov 21, 2022

cible Nov 22, 2022

ekohl Nov 22, 2022

cible commented Nov 23, 2022

bastelfreak commented Jan 15, 2023

		@@ -0,0 +1,3 @@
		---
		letsencrypt::plugin::dns_gandi::package_provider: apt

	is_expected.not_to compile
	is_expected.to compile.and_raise_error(/A Matcher For The Error/)

Add support for the Certbot Gandi plugin #295

Are you sure you want to change the base?

Add support for the Certbot Gandi plugin #295

Conversation

cible commented Aug 26, 2022

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treydock commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cible commented Nov 23, 2022

bastelfreak commented Jan 15, 2023