From 2c722900e55d89a910b49e2a32d04d800402f545 Mon Sep 17 00:00:00 2001 From: Kenyon Ralph Date: Tue, 21 Nov 2023 10:53:15 -0800 Subject: [PATCH 1/3] keyring: rename parameters to remove redundancy We don't need apt::keyring::keyring_thing, it should just be apt::keyring::thing. --- REFERENCE.md | 18 +++++++++--------- manifests/keyring.pp | 24 ++++++++++++------------ manifests/source.pp | 10 +++++----- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/REFERENCE.md b/REFERENCE.md index 92d1b0374a..ad1b87092d 100644 --- a/REFERENCE.md +++ b/REFERENCE.md @@ -660,15 +660,15 @@ apt::source { 'puppet8-release': The following parameters are available in the `apt::keyring` defined type: -* [`keyring_dir`](#-apt--keyring--keyring_dir) -* [`keyring_filename`](#-apt--keyring--keyring_filename) -* [`keyring_file`](#-apt--keyring--keyring_file) -* [`keyring_file_mode`](#-apt--keyring--keyring_file_mode) +* [`dir`](#-apt--keyring--dir) +* [`filename`](#-apt--keyring--filename) +* [`file`](#-apt--keyring--file) +* [`mode`](#-apt--keyring--mode) * [`source`](#-apt--keyring--source) * [`content`](#-apt--keyring--content) * [`ensure`](#-apt--keyring--ensure) -##### `keyring_dir` +##### `dir` Data type: `Stdlib::Absolutepath` @@ -676,7 +676,7 @@ Path to the directory where the keyring will be stored. Default value: `'/etc/apt/keyrings'` -##### `keyring_filename` +##### `filename` Data type: `String[1]` @@ -684,15 +684,15 @@ Optional filename for the keyring. It should also contain extension along with t Default value: `$name` -##### `keyring_file` +##### `file` Data type: `Stdlib::Absolutepath` File path of the keyring. -Default value: `"${keyring_dir}/${keyring_filename}"` +Default value: `"${dir}/${filename}"` -##### `keyring_file_mode` +##### `mode` Data type: `Stdlib::Filemode` diff --git a/manifests/keyring.pp b/manifests/keyring.pp index 83af558257..407ecb1f3d 100644 --- a/manifests/keyring.pp +++ b/manifests/keyring.pp @@ -14,16 +14,16 @@ # } # } # -# @param keyring_dir +# @param dir # Path to the directory where the keyring will be stored. # -# @param keyring_filename +# @param filename # Optional filename for the keyring. It should also contain extension along with the filename. # -# @param keyring_file +# @param file # File path of the keyring. # -# @param keyring_file_mode +# @param mode # File permissions of the keyring. # # @param source @@ -36,15 +36,15 @@ # Ensure presence or absence of the resource. # define apt::keyring ( - Stdlib::Absolutepath $keyring_dir = '/etc/apt/keyrings', - String[1] $keyring_filename = $name, - Stdlib::Absolutepath $keyring_file = "${keyring_dir}/${keyring_filename}", - Stdlib::Filemode $keyring_file_mode = '0644', + Stdlib::Absolutepath $dir = '/etc/apt/keyrings', + String[1] $filename = $name, + Stdlib::Absolutepath $file = "${dir}/${filename}", + Stdlib::Filemode $mode = '0644', Optional[Stdlib::Filesource] $source = undef, Optional[String[1]] $content = undef, Enum['present','absent'] $ensure = 'present', ) { - ensure_resource('file', $keyring_dir, { ensure => 'directory', mode => '0755', }) + ensure_resource('file', $dir, { ensure => 'directory', mode => '0755', }) if $source and $content { fail("Parameters 'source' and 'content' are mutually exclusive") } elsif ! $source and ! $content { @@ -53,9 +53,9 @@ case $ensure { 'present': { - file { $keyring_file: + file { $file: ensure => 'file', - mode => $keyring_file_mode, + mode => $mode, owner => 'root', group => 'root', source => $source, @@ -63,7 +63,7 @@ } } 'absent': { - file { $keyring_file: + file { $file: ensure => $ensure, } } diff --git a/manifests/source.pp b/manifests/source.pp index e1793cea48..0dc104f705 100644 --- a/manifests/source.pp +++ b/manifests/source.pp @@ -183,11 +183,11 @@ # Modern apt keyrings elsif $_key =~ Hash and $_key['name'] { apt::keyring { $_key['name']: - ensure => $_key_ensure, - content => $_key['content'], - source => $_key['source'], - keyring_filename => $_key['filename'], - before => $_before, + ensure => $_key_ensure, + content => $_key['content'], + source => $_key['source'], + filename => $_key['filename'], + before => $_before, } # TODO replace this block with a reference to the apt::keyring's final filename/full_path if $_key['filename'] { From 094bfdb5b20613f35b6c8490d380211f5a776b87 Mon Sep 17 00:00:00 2001 From: Kenyon Ralph Date: Tue, 21 Nov 2023 12:01:22 -0800 Subject: [PATCH 2/3] keyring: remove the file param Being able to provide the dir, filename, and full file path makes the logic too complicated for users of this defined type. We should only allow providing the filename and dir, and then compute the full path from those. Otherwise, someone could pass all three parameters, in which case the filename and dir would be ignored, which could be confusing. --- REFERENCE.md | 9 --------- manifests/keyring.pp | 6 ++---- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/REFERENCE.md b/REFERENCE.md index ad1b87092d..8439572b3e 100644 --- a/REFERENCE.md +++ b/REFERENCE.md @@ -662,7 +662,6 @@ The following parameters are available in the `apt::keyring` defined type: * [`dir`](#-apt--keyring--dir) * [`filename`](#-apt--keyring--filename) -* [`file`](#-apt--keyring--file) * [`mode`](#-apt--keyring--mode) * [`source`](#-apt--keyring--source) * [`content`](#-apt--keyring--content) @@ -684,14 +683,6 @@ Optional filename for the keyring. It should also contain extension along with t Default value: `$name` -##### `file` - -Data type: `Stdlib::Absolutepath` - -File path of the keyring. - -Default value: `"${dir}/${filename}"` - ##### `mode` Data type: `Stdlib::Filemode` diff --git a/manifests/keyring.pp b/manifests/keyring.pp index 407ecb1f3d..d42f4e3c21 100644 --- a/manifests/keyring.pp +++ b/manifests/keyring.pp @@ -20,9 +20,6 @@ # @param filename # Optional filename for the keyring. It should also contain extension along with the filename. # -# @param file -# File path of the keyring. -# # @param mode # File permissions of the keyring. # @@ -38,7 +35,6 @@ define apt::keyring ( Stdlib::Absolutepath $dir = '/etc/apt/keyrings', String[1] $filename = $name, - Stdlib::Absolutepath $file = "${dir}/${filename}", Stdlib::Filemode $mode = '0644', Optional[Stdlib::Filesource] $source = undef, Optional[String[1]] $content = undef, @@ -51,6 +47,8 @@ fail("One of 'source' or 'content' parameters are required") } + $file = "${dir}/${filename}" + case $ensure { 'present': { file { $file: From 5b78cf7f33fbc87fdf658405d97a4b9d7ea08d5d Mon Sep 17 00:00:00 2001 From: Kenyon Ralph Date: Tue, 21 Nov 2023 11:48:36 -0800 Subject: [PATCH 3/3] source: allow passing all apt::keyring attributes This uses the same logic as apt::keyring itself, and what I think would be the least surprising logic: 1. Passing both `dir` and `filename`: you have provided the full path, so that is used in the signed-by setting. 2. Passing just the `filename` means you want to use the default dir, /etc/apt/keyrings. 3. Passing just the `dir` means you want to use the given directory, and the `name` attribute as the filename. 4. Passing only `name` means use `name` with the default dir. --- manifests/source.pp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/manifests/source.pp b/manifests/source.pp index 0dc104f705..de9eeb98dc 100644 --- a/manifests/source.pp +++ b/manifests/source.pp @@ -186,14 +186,20 @@ ensure => $_key_ensure, content => $_key['content'], source => $_key['source'], + dir => $_key['dir'], filename => $_key['filename'], + mode => $_key['mode'], before => $_before, } - # TODO replace this block with a reference to the apt::keyring's final filename/full_path - if $_key['filename'] { - $_list_keyring = $_key['filename'] + + $_list_keyring = if $_key['dir'] and $_key['filename'] { + "${_key['dir']}${_key['filename']}" + } elsif $_key['filename'] { + "/etc/apt/keyrings/${_key['filename']}" + } elsif $_key['dir'] { + "${_key['dir']}${_key['name']}" } else { - $_list_keyring = "/etc/apt/keyrings/${_key['name']}" + "/etc/apt/keyrings/${_key['name']}" } } } else {