syslinux package got split-up in Debian/jessie #132
Conversation
|
I'm trying very hard to come up with a less ugly solution. One could be to rewrite define foreman_proxy::tftp::hosted_file(
$target_root,
$target_filename = undef,
$source_file = $title,
) {
$filename = pick($target_filename, basename($source_file))
file {"${target_root}/${filename}":
ensure => 'present',
source => $source_file,
}
}Then call it with: # Would be in params.pp per OS
$foreman_proxy::hosted_files = ['/usr/lib/syslinux/memdisk', '/usr/lib/PXELINUX/pxelinux.0', '/usr/lib/syslinux/modules/bios/chain.c32', '/usr/lib/syslinux/modules/bios/menu.c32']
foreman_proxy::tftp::hosted_file { $foreman_proxy::hosted_files:
target_root => $foreman_proxy::tftp_root,
}I don't know if it's better or not, but it keeps the number of variables down. |
|
Feels a little less 💩 |
| $tftp_syslinux_root = '/usr/lib/syslinux' | ||
| case $::osfamily { | ||
| 'Debian': { | ||
| if ($::operatingsystem == 'Debian') and ($::operatingsystemrelease =~ /^8/) { |
There was a problem hiding this comment.
Maybe for future readyness do a versioncmp to see if it's 8 or greater?
There was a problem hiding this comment.
We don't know yet, how this will be in Debian/stretch.
|
I'm not sure we should consider sync_file a public API, although I'll admit we haven't marked it as private. (We should definitely mark copy_file as private in the comments if we're adding it, so we don't get this again.) |
|
And I think we need some tests, as the syslinux_filenames parameter issue would have been picked up :) |
| $source_file = $title, | ||
| ) { | ||
| private() | ||
| $filename = inline_template('<%= File.basename(source_file) %>') |
There was a problem hiding this comment.
Maybe add a TODO that the newest stdlib will have this?
|
@domcleal what do you think about dropping the parameters? It'd mean a major version bump. We could leave them in and have a warning so we break less. |
|
|
||
| case $::osfamily { | ||
| 'Debian': { | ||
| if ($::operatingsystem == 'Debian') and ($::operatingsystemrelease =~ /^8/) { |
There was a problem hiding this comment.
versioncmp so it can also match newer versions?
|
@ekohl I'm not sure.. I don't mind if we need to bump it I suppose, though it doesn't seem a particularly important or large change. The alternative is to leave them both in but with undef defaults, leave sync_file in and then add a deprecation warning if they're ever set, or sync_file is called - which I prefer. |
|
I agree that leaving them in and adding a warning is better. That does mean every installer user who uses an old answer file will see deprecation warnings if they enable verbose output, but that's fine. |
| @@ -13,8 +13,7 @@ | |||
| recurse => true; | |||
| } | |||
|
|
|||
| foreman_proxy::tftp::sync_file{$foreman_proxy::tftp_syslinux_files: | |||
There was a problem hiding this comment.
I think we need to keep this if tftp_syslinux_files is set?
There was a problem hiding this comment.
and print a deprecation warning here too
There was a problem hiding this comment.
Certainly we should print a warning, but I'm not sure about syncing them as well. In a pure puppet world I would agree but with the installer and old answer files I'm afraid about duplicate syncs. If a user upgraded their debian as well it would lead to errors. OTOH that's a bit of an edge case.
There was a problem hiding this comment.
Maybe put the new block in an "else" clause of if $foreman_proxy::tftp_syslinux_files?
|
Looks correct to me! 👍 I might give it a quick test first, as we're missing coverage in this area. |
|
Should we add Debian 8 to the supported OS list, or wait? Are there other issues in this module? |
| require => Class['tftp::install']; | ||
| } | ||
|
|
||
| warning('foreman_proxy::tftp_syslinux_files is deprecated and will be removed') |
There was a problem hiding this comment.
.. deprecated in favor of tftp_syslinux_filenames ...
There was a problem hiding this comment.
oh, and please add a note to the init.pp parameter documentation to say the old parameters are deprecated.
|
Updated. I'll have to test this again on a fresh box, but it worked fine on jessie. |
| # deprecated, see $tftp_syslinux_filenames) | ||
| # type:array | ||
| # | ||
| # $tftp_syslinux_filenames:: Syslinux files to install on TFTP (pull paths) |
There was a problem hiding this comment.
Er, this parameter doesn't actually exist, it's just in params :)
|
Now with minimal corrections and one minimal test. I expected when the facts are set accordingly, e.g. |
|
domcleal@eae9638 works, feel free to squash it in. |
|
Thanks @domcleal, I extended it a bit, seems I'm having some problems with the tests, as others also fail for no reason... Anyway, I hope we're good to go with this one now. 🙏 |
| @@ -133,9 +133,11 @@ | |||
| # | |||
| # $tftp_listen_on:: TFTP proxy to listen on https, http, or both | |||
| # | |||
| # $tftp_syslinux_root:: Directory that hold syslinux files | |||
| # $tftp_syslinux_filenames:: Syslinux files to install on TFTP (pull paths) | |||
|
👍 |
| @@ -133,9 +133,11 @@ | |||
| # | |||
| # $tftp_listen_on:: TFTP proxy to listen on https, http, or both | |||
| # | |||
| # $tftp_syslinux_root:: Directory that hold syslinux files | |||
There was a problem hiding this comment.
This parameter should stay in but marked as deprecated.
|
👍 |
|
Merged, thanks @mmoll! |
I hope somebody is having a less humble way to do this :)