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

Clundquist/dpkg install options #9202

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

clundquist-stripe
Copy link

I have a custom .deb that is a little wonky and installs into / so we have /memcached.
We need to set --instdir so that it ends up in the place we expect, however, currently install_options has no effect for dpkg.

I want this to work as you expect:

$memcached_debian_path = "/path/to/memcached/${memcached_debian}"
package { 'memcached':
    ensure          => latest,
    provider        => dpkg,
    source          => $memcached_debian_path,
    install_options => ['--instdir=/path/to/memcached'],
    # ...

Currently, install_options is ignored.

Debug: Executing '/usr/bin/dpkg-query -W --showformat '${Status} ${Package} ${Version}\n' memcached'
Debug: Executing 'dpkg --set-selections'
Debug: Executing '/usr/bin/dpkg --force-confold -i /path/to/memcached/memcached_1.6.19b.deb'

This should honor install_options so that my resource works as intended.

This change is difficult for me to test as we are behind on puppet versions, so I'll need to rely on CI testing and additional testing from maintainers.

@clundquist-stripe clundquist-stripe requested a review from a team as a code owner January 8, 2024 22:11
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@CLAassistant
Copy link

CLAassistant commented Jan 8, 2024

CLA assistant check
All committers have signed the CLA.

if @resource[:uninstall_options]
flags += @resource[:uninstall_options]
end
dpkg *flags, @resource[:name]
end

def purge
Copy link
Author

Choose a reason for hiding this comment

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

I wasn't sure if we want :uninstall_options for purge too, or if that would make things more difficult.

Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

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

Hi @clundquist-stripe thanks for your contribution. This line:

flags += @resource[:uninstall_options]

will append options without any spaces between them. Probably easier to switch flags to an array like args is in the install case and then splat flags like you're doing.

@clundquist-stripe
Copy link
Author

Great catch @joshcooper !

@joshcooper
Copy link
Contributor

Thanks! Sorry forgot to mention earlier could you squash your commits and add tests? I just merged a pacman provider change that is similar to this one.

@joshcooper
Copy link
Contributor

Hi @clundquist-stripe could you squash your commits?

@clundquist-stripe
Copy link
Author

can do!
I haven't added tests just yet though :(

[dpkg] add uninstall_options feature

[dpkg] uninstall_options, flags -> args array

[dpkg] install_options, is an array, += args
@joshcooper
Copy link
Contributor

Thanks @clundquist-stripe! Could you add some unit tests to make sure install and uninstall options are getting passed through to the underying dpkg commands as expected? For example, see how this is done for yum:

it { is_expected.to be_install_options }

describe 'with install_options' do

@joshcooper joshcooper added the enhancement New feature or request label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants