Skip to content

Zypper provider install options#2067

Closed
darix wants to merge 2 commits intopuppetlabs:masterfrom
darix:zypper-provider-install-options
Closed

Zypper provider install options#2067
darix wants to merge 2 commits intopuppetlabs:masterfrom
darix:zypper-provider-install-options

Conversation

@darix
Copy link
Contributor

@darix darix commented Nov 14, 2013

The old code would convert
install_options => { '--from' => 'foobar' }

into a zypper call

zypper install "--from 'foobar'"

which doesnt work.

new code does
zypper install --from=foobar

install_options => { '--from' => 'foobar' }

into a zypper call

zypper install "--from 'foobar'"

which doesnt work.

new code does
zypper install --from=foobar
@puppetcla
Copy link

Waiting for CLA signature by @darix

@darix - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html

@darix
Copy link
Contributor Author

darix commented Nov 22, 2013

we signed the CLA as company now.

@MosesMendoza
Copy link
Contributor

@darix according to the zypper documentation SLES 11, the format of the option should be space-separated:

--from <alias|name|#|URI>
    Select  packages  from  specified  repository.  If strings specified as arguments to the install command match packages in repositories specified in this option, they will be marked for installation. This option  currently  implies  --name,  but allows using wildcards for specifying packages.

Is this issue at hand actually the single quotes around the value?

@darix
Copy link
Contributor Author

darix commented Feb 4, 2014

I actually use that code on sles.

Copy link
Contributor

Choose a reason for hiding this comment

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

If v has spaces in it, should that be quoted as a single argument? Is the important bit using the = to join the option and argument?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, this fix is incorrect. We're sorting the keys here, so v is always nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we handle automatically applying quotes in lib/puppet/parameter/package_options.rb if a value has a space in it, so adding quotes will cause it to be double quoted.

@MosesMendoza
Copy link
Contributor

@adrienthebo I believe the important bit is not providing literal quotes around the option+argument pair "--from REPO".

Doing zypper --from 'repo' and zypper --from='repo' should be equivalent

@ffrank
Copy link
Contributor

ffrank commented Feb 5, 2014

We talked about this briefly during the triage. We disbelieve those quotes are actually in effect.

I tried to reproduce, but had a hard time getting master to run on a SLES12 box, unfortunately.

We should probably be discussing this on Jira.

@peterhuene
Copy link
Contributor

I was able to reproduce on SLES 11: puppet apply -e 'package { "vim": ensure => present, install_options => { "--from" => "1" } }' fails with Unknown option '--from 1'. This appears to be due to some weirdness with Kernel#exec and zypper. If we pass --from 1, we get the "unknown option" error from zypper. If we pass --from=1, zypper respects the option.

I've verified that this fix (when itself is fixed, since v is always nil in the requested change) does cause zypper to properly respect a install_options hash. I'll clean up, add some test cases, and merge in.

@peterhuene
Copy link
Contributor

@darix Thanks for your contribution. I've squashed and amended your commits and then added a commit for spec fixes. Please see GH-2441. I'm going to close this PR in favor of that one. Thanks again for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants