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
Fix version handling for remi repo #510
base: master
Are you sure you want to change the base?
Conversation
There's some weird stuff going on with |
Hi @Rathiosl thanks for providing this PR. Can you please add acceptance tests for the change to use the remi repository? Are you interested in cleaning up the class (probably in a seperate PR)? Since PHP 5.6 is EOL we could probably remove all this legacy code. |
I wouldn't mind cleaning stuff up. I'd argue I'm pretty good at that. Getting started with acceptance testing using
Over at beaker-vagrant I find: Couple questions:
Edit:
|
Hi. For most tests we generate the nodesets on the fly with beaker hostgenerator. Also on travis and locally we use the docker backend and not vagrant. You can take a look at the Lines 26 to 27 in 64bbc0b
so the command to test Ubuntu with docker would be:
Or for CentOS 7:
If you want to use vagrant, you need to add the The vagrant command should be (I don't have Vagrant by hand to verify this):
Sorry for the trouble. I will update our Gemfiles later. Let me know if you've furthe questions. |
CONTRIBUTING.md contains instructions for how to run acceptance tests using vagrant, but Beaker v4+ has removed dependencies on Beaker hypervisor gems. We have to install them via Gemfile in each module now. This should make it easier for newbies to get started with acceptance testing as well. Especially in environments like WSL on Win10 where it's much easier to get Vagrant working with VirtualBox than getting a Docker environment running.
@bastelfreak Thanks! That helped me get things sorted out. Much appreciated. :) This should be ready for merge now, if everything looks okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me from a syntactical and logical POV, cannot vouchsafe functionality, though. :)
Just wanted to chime in and see if what we wrote a while ago may be of use. We found (at least at the time) we had to fix a few things when switching from the stock php repo to remi.
We wrapped our php class around this module’s stuff so not everything here may be necessary anymore but I wanted to share in case any of that might still be needed. class coldfront::php {
include ::php
$version = lookup('php::upgrade', {value_type => String, default_value => '56'})
if $version != 'default' {
class {"coldfront::php::phpupdate":
version => $version,
}
}
$php_extensions = lookup("php::extensions", {value_type => Hash, merge => 'deep', default_value => {}})
$php_config_root_ini = pick_default($::php::config_root_ini, $::php::params::config_root_ini)
# Create the certs
$php_extensions.each |$key, $value| {
# Get the extension name.
# @see https://github.com/voxpupuli/puppet-php/blob/master/manifests/extension.pp#L65
if (!empty($value['so_name'])) {
$ext_name = downcase($value['so_name'])
}
else {
$ext_name = downcase($key)
}
# Check for the ini prefix.
if (!empty($value['ini_prefix'])) {
$ini_prefix = $value['ini_prefix']
}
else {
$ini_prefix = ''
}
exec {"php-conf-clean-${$key}":
cwd => $php_config_root_ini,
command => "find . -type f \( -name '*-${ext_name}.ini' -o -name '${ext_name}.ini' \) ! -name '${ini_prefix}${ext_name}.ini' -delete",
onlyif => "test \$(find . -type f \( -name '*-${ext_name}.ini' -o -name '${ext_name}.ini' \) ! -name '${ini_prefix}${ext_name}.ini' | wc -l) -ne 0",
path => ['/bin', '/usr/bin'],
require => Class['::php'],
}
}
# Install the right apache module for PHP if required.
$classes = lookup('classes', {value_type => Array, default_value => []})
if 'coldfront::role::webserver::apache' in $classes {
# Fix some ordering issues with puppet. It could be that
# coldfront::role::webserver::apache hasn't been applied yet.
# Ensure it is.
require coldfront::role::webserver::apache
class { 'apache::mod::php':
php_version => $version[0],
}
}
}
class coldfront::php::composer {
class {"::composer":
auto_update => true,
build_deps => false,
require => [Class['coldfront::php'], Class['::php']],
}
}
class coldfront::php::phpupdate (
$version = '56',
) {
# @todo add regex calidation on $version variable (always two digits).
if $::osfamily == 'redhat' {
# @see https://github.com/voxpupuli/puppet-php/blob/master/manifests/repo/redhat.pp#L13
$releasever = $facts['os']['name'] ? {
/(?i:Amazon)/ => '6',
default => '$releasever', # Yum var
}
yumrepo { 'remi':
descr => 'Remi\'s RPM repository for Enterprise Linux $releasever - $basearch',
mirrorlist => "https://rpms.remirepo.net/enterprise/${releasever}/remi/mirror",
enabled => 1,
gpgcheck => 1,
gpgkey => 'https://rpms.remirepo.net/RPM-GPG-KEY-remi',
priority => 1,
}
yumrepo { "remi-php${version}":
descr => 'Remi\'s PHP RPM repository for Enterprise Linux',
mirrorlist => "https://rpms.remirepo.net/enterprise/${releasever}/php${version}/mirror",
enabled => 1,
gpgcheck => 1,
gpgkey => 'https://rpms.remirepo.net/RPM-GPG-KEY-remi',
priority => 1,
before => Class['::php'],
require => Yumrepo['remi'],
}
}
} |
Dear @Rathios, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
Pull Request (PR) description
beaker-vagrant
to gemfile (see details in commit message for why)Note: This changes the default version string of
$php::globals::globals_php_version
from5.x
to5.6
on non-Debian/Ubuntu because we really should use sane version strings in such variables so we can re-use them elsewhere (like inphp::repo::redhat
for remi repo). I anticipate the versioning inphp::globals
will be cleaned up eventually. (I'll volunteer too.) It doesn't look like this change will have any adverse effects.If merged, this PR obsoletes the following PRs: (Sorry, I'm impatient and they don't look like they're going anywhere)
This Pull Request (PR) fixes the following issues
Fixes #480