Support paramatized yum::versionlock #169

traylenator · 2020-05-06T22:20:16Z

Pull Request (PR) description

Support paramatized yum::versionlock

Motivated by the change of format to the versionlock file with DNF systems
change yum::versionlock so it supports parameters version,
release, epoch and arch. If version is specified the namevar is now the package name.

So on a DNF based systemd (e.g CentOS 8) the following must be used.

yum::versionlock{'bash':
  version => '3.4',
}

results in a version lock of on a dnf based system

bash-0:3.4-*

and on a yum based system of

0:bash-3.4-*

A fuller example with all parameters set.

yum::versionlock{'bash':
  version  => '5.4',
  release  => 'alpha-rc3',
  epoch    => 8,
  arch     => 'noarch'
}

results in a version lock on a DNF based system of

bash-8:5.4-alpha-rc3.noarch

The same mechanism on YUM based systems (e.g CentOS 7) can be used.
To specify this new machanism at least a version should be set.

yum::versionlock{'bash':
  version  => '5.4',
  release  => 'alpha-rc3',
  epoch    => 8,
  arch     => 'noarch'
  future   => true,
}

would results in

8:bash-5.4-alpha-rc3.noarch

as used by yum. If version is left default undef then the old mechanism is used so this
is backwards compatable. e.g

yum::versionlock{"0:bash-3.4-7.el8.x86_64"}

is still valid

This Pull Request (PR) fixes the following issues

Fixes #150

traylenator · 2020-05-06T22:30:06Z

Currently if

yum::versionlock{'0:bash-4.1.2-9.el6_2.x86_64':}

is used this compiles on CentOS 8 but is almost certainly not what was wanted.
Can we make the assumption that package names never contain : . It does seem to be case
for Fedora.

In principle could ditch the future parameter in this case and make the choice of mode
automatic on 7.

traylenator · 2020-05-06T23:30:13Z

I tried to enable CentOS 8 accept tests but the framework hangs early.

vchepkov · 2020-05-07T12:05:11Z

IMHO, future attribute is unnecessary. resource should make decision what format to use based on package provider. 'dnf' provider should use new format

manifests/versionlock.pp

traylenator · 2020-05-07T12:30:33Z

IMHO, future attribute is unnecessary. resource should make decision what format to use based on package provider. 'dnf' provider should use new format

It is in the sense you can ignore it.

On 8 you get the new behaviour.
On 7 you get the old behaviour.

You can choose to get the new behaviour on 7 as well with future => true

$package_provider is is totally used to decide a yum or dnf style format.

README.md

REFERENCE.md

vchepkov · 2020-05-07T13:38:40Z

Not sure why you would ever set 'future' on EL7, since it will render configuration invalid.

Ah, I think I got what you are trying to do. IMHO, instead of adding 'future' attribute, why not deduct it from the fact that any of the new attributes are set instead of undef?

README.md

traylenator · 2020-05-11T11:25:29Z

Sorry for delay - long weekend here.

Ah, I think I got what you are trying to do. IMHO, instead of adding 'future' attribute, why not deduct it from the fact that any of the new attributes are set instead of undef?

My first iteration of this I set arch, release version to undef as default and then used one of them being set to infer we were in modern mode. I changed as I prefer the * defaults as its more useful in the future - a better final interface.

Can I draw your attention to the : in package name comment above as I think that is the best way to drop the need or desire for the a future parameter on 7.

Ah, I think I got what you are trying to do.

Just to confirm I my motivation was to avoid maintaining

if versioncmp($facts['os']['release']['version'],'7') <= 0 {
   yum::versionlock('0:bash-1.2.3.el6':}
} else {
  yum::versionlock('bash':
    version => '1.2.3',
    release  => 'el6',
 }
}

Instead folk can just do for 6 , 7 and 8

yum::versionlock('bash':
    version => '1.2.3',
    release  => 'el6',
    future    => true,
}

with this patch as it stands people have the choice of doing either above.

traylenator · 2020-05-11T11:26:59Z

In principle this patch supports someone installing dnf on CentOS 7 as well which is now available. Can't comment on the rest of the module mind you.

vchepkov · 2020-05-11T11:36:52Z

I understand. IMHO, you can set default value for version to undef and use that as indicator of the future format instead.

vchepkov · 2020-05-11T11:50:44Z

In principle this patch supports someone installing dnf on CentOS 7 as well which is now available. Can't comment on the rest of the module mind you.

Oh, haven't realized that. It's not in any major repositories. In this case maybe parameter should be Enum['dnf','yum'] $format = 'yum' ?

traylenator · 2020-05-11T12:02:37Z

I understand. IMHO, you can set default value for version to undef and use that as indicator of the future format instead.

Seems sensibe. Indeed I can't think of use case where you would set release to something and leave version as * and even if you did want that you could change version to *.

I'll make those changes.

Oh, haven't realized that. It's not in any major repositories. In this case maybe parameter should be Enum['dnf','yum'] $format = 'yum' ?

I think we would have to assume that package_provider fact would be changed to dnf on dnf installed on 7 machine. Certainly assume it until someone actually tries it. Not something I will do until CentOS 6 is dead and out the window.

vchepkov · 2020-05-11T12:21:55Z

If this new parameter is just to accommodate EL7 users with dnf installed, then disregard my last comment.

package_provider comes from stdlib module, but it just uses logic from the puppet itself

https://github.com/puppetlabs/puppet/blob/master/lib/puppet/provider/package/dnf.rb#L31-L34

According to it, package_provider will be set to yum on EL7

vchepkov · 2020-05-11T13:02:26Z

Another option is to drop the old format and make this incompatible change release.
The old code is bad, since you are allowed to 'lock' multiple versions of the same package without ever noticing.

vchepkov · 2020-05-11T14:22:51Z

You should probably remove that future attribute, since you don't use it anymore.
Other than that looks good!

ekohl

The actual code change looks ok. I'd like to see some tests for the type aliases. https://rspec-puppet.com/documentation/type_aliases/ makes this very easy to test. I'm mostly interested because I don't remember if Pattern needs ^ and $ or if it's a full match so the it { is_expected.to allow_value('-a') for RpmVersion would be interesting.

REFERENCE.md

manifests/versionlock.pp

types/rpmarch.pp

types/rpmepoch.pp

types/rpmname.pp

traylenator · 2020-05-12T05:33:05Z

@ekohl Indeed the regexs on version, release , ... absolutely needed a ^ and $.

Have added type_aliases tests now.
Have improved the rendering of type string data now I hope but Remove nested code blocks from string docs #171 is needed first to stop current rendering failure to really confirm.

vchepkov · 2020-05-12T08:43:08Z

puppetlabs/stdlib just recently went through replacing all regex with ^ and $ to \A and \z instead
epoch can't be '*', in fact you added test in line 8 in yum_rpmepoch_spec.rb that it can't be star, but Variant allows it
line with assert_type(Yum::RpmVersion is redundant, this is already done by argument type validation
I am not sure where the symbols that are allowed by type Yum::RpmName are coming from.
the referenced spec file doesn't mention it, but according https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/ only few symbols are allowed in the name

ekohl · 2020-05-12T08:49:34Z

puppetlabs/stdlib just recently went through replacing all regex with ^ and $ to \A and \z instead

Just to help me understand: what's the reason for this?

vchepkov · 2020-05-12T08:55:15Z

puppetlabs/stdlib just recently went through replacing all regex with ^ and $ to \A and \z instead

Just to help me understand: what's the reason for this?

It was just an observation. I presume it's to avoid allowing a multiline string ?
Here is the commit puppetlabs/puppetlabs-stdlib@b74585b

traylenator · 2020-05-12T10:56:24Z

puppetlabs/stdlib just recently went through replacing all regex with ^ and $ to \A and \z instead

updated.

epoch can't be '*', in fact you added test in line 8 in yum_rpmepoch_spec.rb that it can't be star, but Variant allows it
line with assert_type(Yum::RpmVersion is redundant, this is already done by argument type validation

That makes sense to me. RPM's do not support arch of * but versionlock files do
support a * for arch.

I am not sure where the symbols that are allowed by type Yum::RpmName are coming from.
the referenced spec file doesn't mention it, but according https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/ only few symbols are allowed in the name

I think they were lifted from the previous versionlock type.

Above link is a guideline rather than enforced. If I try and make the RPM with a crazy name.

Name: p.u_p+p%%e{t}ag-ent

I do indeed get an RPM

/home/steve/rpmbuild/SRPMS/p.u_p+p%e{t}ag-ent-5.5.19.5.g340c40ba-1.fc32.src.rpm

:-)

The * does cause an exception so I'll remove the * as an option from name now.

traylenator · 2020-05-12T11:07:48Z

line with assert_type(Yum::RpmVersion is redundant, this is already done by argument type validation

agreed.

traylenator · 2020-05-12T12:05:28Z

l> line with assert_type(Yum::RpmVersion is redundant, this is already done by argument type validation

agreed.

It is needed afterall. The argument type allows undef for a yum based machine but on a
dnf based machine version must be set.

@vchepkov I believe I have addressed your comments.

vchepkov · 2020-05-12T12:49:28Z

It is needed afterall. The argument type allows undef for a yum based machine but on a
dnf based machine version must be set.

IMHO, you don't need it, but it's not important, it's just a matter of preference.
In your case value of undef is unacceptable, the type assertion is not necessary.
also, I wouldn't ascertain $version twice. pseudo code:

  if $version {
   # Variable is defined, must be a new style and pattern validated by type
    case $facts['package_provider'] {
       'yum': { yum pattern }
       'dnf': { dnf pattern }
       default: { fail not supported yet}
    }
  } else {
  # old style, assert name type
 }

vchepkov · 2020-05-12T12:53:54Z

That makes sense to me. RPM's do not support arch of * but versionlock files do
support a * for arch.

I was talking about epoch, which can't be '*'

[root@puppet ~]# cat /etc/yum/pluginconf.d/versionlock.list 
puppet-agent-*:6.14.0-1.el8.*
[root@puppet ~]# yum update
Last metadata expiration check: 0:03:32 ago on Tue 12 May 2020 12:49:55 PM UTC.
Versionlock plugin: could not parse pattern: puppet-agent-*:6.14.0-1.el8.*

traylenator · 2020-05-12T14:00:19Z

That makes sense to me. RPM's do not support arch of * but versionlock files do
support a * for arch.

I was talking about epoch, which can't be '*'
[root@puppet ~]# cat /etc/yum/pluginconf.d/versionlock.list 
puppet-agent-*:6.14.0-1.el8.*
[root@puppet ~]# yum update
Last metadata expiration check: 0:03:32 ago on Tue 12 May 2020 12:49:55 PM UTC.
Versionlock plugin: could not parse pattern: puppet-agent-*:6.14.0-1.el8.*

traylenator · 2020-05-13T06:59:50Z

I was talking about epoch, which can't be '*'

epoch is now just an Integer[0]

traylenator · 2020-05-14T13:13:29Z

Have squashed commits , will merge Monday if no more comment.

REFERENCE.md

Motivated by the change of format to the versionlock file with DNF systems change `yum::versionlock` so it supports parameters `version`, `release`, `epoch` and `arch`. If version is specified the namevar is now the package name. So on a DNF based systemd (e.g CentOS 8) the following must be used. ```puppet yum::versionlock{'bash': version => '3.4', } ``` results in a version lock of on a dnf based system ``` bash-0:3.4-* ``` and on a yum based system of ``` 0:bash-3.4-* ``` A fuller example with all parameters set. ```puppet yum::versionlock{'bash': version => '5.4', release => 'alpha-rc3', epoch => 8, arch => 'noarch' } ``` results in a version lock on a DNF based system of ``` bash-8:5.4-alpha-rc3.noarch ``` The same mechanism on YUM based systems (e.g CentOS 7) can be used. To specify this new machanism at least a `version` should be set. ```puppet yum::versionlock{'bash': version => '5.4', release => 'alpha-rc3', epoch => 8, arch => 'noarch' future => true, } ``` would results in ``` 8:bash-5.4-alpha-rc3.noarch ``` as used by yum. If `version` is left default `undef` then the old mechanism is used so this is backwards compatable. e.g ```puppet yum::versionlock{"0:bash-3.4-7.el8.x86_64"} ``` is still valid Fixes: 150

spec/defines/versionlock_spec.rb

Use the new versionlock specification from voxpupuli#169 on CentOS 8. Also test the new specification on 7 and 8 also. Finally test that if samba-devel is filtered at an impossible version it is no longer available. This for the yum case probably assumes that samba-devel is not actually installed.

Correct acceptence tests for DNF (8) system Use the new versionlock specification from voxpupuli#169 on CentOS 8. Also test the new specification on 7 and 8 also. Finally test that if samba-devel is filtered at an impossible version it is no longer available. This for the yum case probably assumes that samba-devel is not

traylenator added needs-tests needs-work not ready to merge just yet labels May 6, 2020

traylenator force-pushed the parameter branch 3 times, most recently from 6278d20 to 882f2e8 Compare May 6, 2020 23:13

traylenator changed the title ~~WIP: Support paramatized yum::versionlock~~ Support paramatized yum::versionlock May 6, 2020

traylenator added bug Something isn't working enhancement New feature or request and removed needs-tests needs-work not ready to merge just yet labels May 6, 2020

traylenator mentioned this pull request May 7, 2020

add versionlock format for dnf #165

Closed

vchepkov reviewed May 7, 2020

View reviewed changes

manifests/versionlock.pp Outdated Show resolved Hide resolved

vchepkov reviewed May 7, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

vchepkov reviewed May 7, 2020

View reviewed changes

REFERENCE.md Outdated Show resolved Hide resolved

vchepkov reviewed May 7, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

traylenator added needs-squash needs-work not ready to merge just yet labels May 11, 2020

dhoppe requested a review from ekohl May 11, 2020 18:46

ekohl reviewed May 11, 2020

View reviewed changes

traylenator mentioned this pull request May 12, 2020

Remove nested code blocks from string docs #171

Merged

traylenator force-pushed the parameter branch from 41d8f8d to a5d5385 Compare May 12, 2020 10:41

traylenator added the tests-fail label May 12, 2020

traylenator closed this May 12, 2020

traylenator reopened this May 12, 2020

bastelfreak approved these changes May 12, 2020

View reviewed changes

traylenator removed the tests-fail label May 13, 2020

traylenator force-pushed the parameter branch from 16dfe2e to deeba81 Compare May 14, 2020 13:13

vchepkov reviewed May 14, 2020

View reviewed changes

REFERENCE.md Outdated Show resolved Hide resolved

traylenator force-pushed the parameter branch from 216031d to b0e1bd2 Compare May 15, 2020 08:44

ekohl approved these changes May 15, 2020

View reviewed changes

spec/defines/versionlock_spec.rb Show resolved Hide resolved

traylenator merged commit aa9b065 into voxpupuli:master May 18, 2020

traylenator mentioned this pull request May 18, 2020

Enable and add CentOS 8 accept tests #172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support paramatized yum::versionlock #169

Support paramatized yum::versionlock #169

traylenator commented May 6, 2020 •

edited

Loading

traylenator commented May 6, 2020 •

edited

Loading

traylenator commented May 6, 2020

vchepkov commented May 7, 2020

traylenator commented May 7, 2020 •

edited

Loading

vchepkov commented May 7, 2020 •

edited

Loading

traylenator commented May 11, 2020 •

edited

Loading

traylenator commented May 11, 2020

vchepkov commented May 11, 2020

vchepkov commented May 11, 2020

traylenator commented May 11, 2020

vchepkov commented May 11, 2020 •

edited

Loading

vchepkov commented May 11, 2020

vchepkov commented May 11, 2020

ekohl left a comment

traylenator commented May 12, 2020

vchepkov commented May 12, 2020 •

edited

Loading

ekohl commented May 12, 2020

vchepkov commented May 12, 2020 •

edited

Loading

traylenator commented May 12, 2020

traylenator commented May 12, 2020

traylenator commented May 12, 2020

vchepkov commented May 12, 2020

vchepkov commented May 12, 2020

traylenator commented May 12, 2020

traylenator commented May 13, 2020

traylenator commented May 14, 2020

Support paramatized yum::versionlock #169

Support paramatized yum::versionlock #169

Conversation

traylenator commented May 6, 2020 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

traylenator commented May 6, 2020 • edited Loading

traylenator commented May 6, 2020

vchepkov commented May 7, 2020

traylenator commented May 7, 2020 • edited Loading

vchepkov commented May 7, 2020 • edited Loading

traylenator commented May 11, 2020 • edited Loading

traylenator commented May 11, 2020

vchepkov commented May 11, 2020

vchepkov commented May 11, 2020

traylenator commented May 11, 2020

vchepkov commented May 11, 2020 • edited Loading

vchepkov commented May 11, 2020

vchepkov commented May 11, 2020

ekohl left a comment

Choose a reason for hiding this comment

traylenator commented May 12, 2020

vchepkov commented May 12, 2020 • edited Loading

ekohl commented May 12, 2020

vchepkov commented May 12, 2020 • edited Loading

traylenator commented May 12, 2020

traylenator commented May 12, 2020

traylenator commented May 12, 2020

vchepkov commented May 12, 2020

vchepkov commented May 12, 2020

traylenator commented May 12, 2020

traylenator commented May 13, 2020

traylenator commented May 14, 2020

traylenator commented May 6, 2020 •

edited

Loading

traylenator commented May 6, 2020 •

edited

Loading

traylenator commented May 7, 2020 •

edited

Loading

vchepkov commented May 7, 2020 •

edited

Loading

traylenator commented May 11, 2020 •

edited

Loading

vchepkov commented May 11, 2020 •

edited

Loading

vchepkov commented May 12, 2020 •

edited

Loading

vchepkov commented May 12, 2020 •

edited

Loading