[PUP-4744] Yumrepo doesn't recognize whitespace delimited reposdir settings in /etc/yum.conf Created: 2015/06/11  Updated: 2016/10/19  Resolved: 2016/01/13

Status: Closed
Project: Puppet
Component/s: Types and Providers
Affects Version/s: PUP 3.8.1, PUP 4.1.0
Fix Version/s: PUP 4.3.2

Type: Bug Priority: Normal
Reporter: Charlie Sharpsteen Assignee: Steve Barlow
Resolution: Fixed Votes: 0
Labels: customer, puppethack, support
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

osfamily => RedHat


Issue Links:
Relates
relates to PUP-2916 yumrepo fails to honour custom reposdir Closed
Template:
Story Points: 1
Sprint: Client 2015-12-30, Client 2016-01-13
CS Priority: Reviewed
Release Notes: Bug Fix
Release Notes Summary: The reposdir setting for yum allows commas, whitespace including newlines, but puppet's yumrepo type only recognized commas and would add the repo definitions to /etc/yum.conf. This fix updates puppet to also recognize whitespace and newlines, creating repo definitions in /etc/yum.conf.d as expected.
QA Contact: Eric Thompson

 Description   

The inifile provider for the Yumrepo type searches various directories when deciding where to create repository definitions. One source of directories is the reposdir setting of /etc/yum.conf. Yum allows this setting to contain a list of directories delimited by either commas or whitespace, however Puppet only uses commas when parsing the setting.

If the reposdir setting contains a whitespace delimited list of directories, then Puppet will almost silently (a DEBUG level message is logged) fall back to creating definitions directly in /etc/yum.conf.

Reproduction Case

  • Install Puppet on a Red Hat system.
  • Add a reposdir setting to /etc/yum.conf which contains a whitespace-delimited list:

  echo 'reposdir=/etc/yum.conf.d /tmp' >> /etc/yum.conf

  • Attempt to create a new repo definition.

Outcome

The definition is added to /etc/yum.conf instead of a new file under /tmp/:

# /opt/puppet/bin/puppet resource yumrepo foo ensure=present descr='foo repo'
Notice: /Yumrepo[foo]/ensure: created
yumrepo { 'foo':
  ensure => 'present',
  descr  => 'foo repo',
}
 
# ls /etc/yum.repos.d/foo.repo
ls: cannot access /etc/yum.repos.d/foo.repo: No such file or directory

Expected Outcome

The definition is created under /tmp/ as it's the last reposdir specified

# /opt/puppet/bin/puppet resource yumrepo foo ensure=present descr='foo repo'
Notice: /Yumrepo[foo]/ensure: created
yumrepo { 'foo':
  ensure => 'present',
  descr  => 'foo repo',
}
 
# ls /tmp/foo.repo 
/tmp/foo.repo



 Comments   
Comment by Josh Cooper [ 2015/12/10 ]

Probably just need to change: https://github.com/puppetlabs/puppet/blob/4.3.1/lib/puppet/provider/yumrepo/inifile.rb#L77

Comment by Kylo Ginsberg [ 2015/12/28 ]

I just went to verify this and the outcome with this fix is different than described above. I think it's correct, but I'd like a sanity check, /cc Peter Souter or Charlie Sharpsteen.

Specifically, it chooses the last, not the first, directory in the reposdir setting. I.e. given the arrangement in the description, it will create /tmp/foo.repo, not /etc/yumrepos.d/foo.repo. I think this is the appropriate thing to do, e.g. see the reasoning in the comment at https://github.com/puppetlabs/puppet/blob/4.2.2/lib/puppet/provider/yumrepo/inifile.rb#L169-L171.

If it is the appropriate thing to do, we might want to flesh out the docstring at https://github.com/puppetlabs/puppet/blob/4.2.2/lib/puppet/provider/yumrepo/inifile.rb#L17-L21 to call out this case.

If not, then we should discuss what the desired behavior is.

Comment by Kylo Ginsberg [ 2016/01/11 ]

Last call for watchers on this. See my comment above. Please respond with objections to the behavior by Tuesday Jan 12. If I don't here otherwise I'll update the description to match the behavior (which, again, I think actually makes sense). Thanks all!!

Comment by Josh Cooper [ 2016/01/11 ]

The behavior seems to be intentional due to PUP-2218 in https://github.com/puppetlabs/puppet/commit/cd4e69f5dafac33740b6c1d711451f8da169cae3#diff-f9fc191a659f726148aafaef592ee558L109. It seems ok to me, but it'd be good to get confirmation from Adrien Thebo

Comment by Adrien Thebo [ 2016/01/11 ]

In Puppet 3.5.0/3.5.1 the yumrepo type was split out into a distinct type and provider, which was a massive overhaul. Moreover that code had almost no effective tests beforehand so we regressed on more than a handful of features during the refactor. It looks like this line (https://github.com/puppetlabs/puppet/blob/3.2.3/lib/puppet/type/yumrepo.rb#L112) originally made either spaces or commas work, but this intent wasn't clearly called out so I suspect that it was accidentally dropped.

+1 for treating commas and spaces as equivalent values for the reposdir setting.

(edit: it's worth noting that per http://linux.die.net/man/5/yum.conf the path separator character isn't specified in the documentation for yum.conf, at least as far as I can tell. We should definitely document this behavior in the code + add tests so that we don't regress on this again.)

(edit 2: it looks like some settings like reposdir and tsflags take a comma or space separated list, while settings like exclude only take space separated lists.)

Comment by Kylo Ginsberg [ 2016/01/12 ]

Thanks for chiming in folks!

Moving this along to Ready for Review, since I did the verification up above.

Generated at Thu Nov 14 01:49:31 PST 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.