Add PHP 7.1 support on Debian #293

fstr · 2017-01-16T23:30:10Z

I've added support for PHP 7.1 on Debian (jessie, stretch).

PHP 7.1.0-5+0~20161222133327.13+jessie~1.gbp46a191 (cli) ( NTS )
Copyright (c) 1997-2016 The PHP Group
Zend Engine v3.1.0-dev, Copyright (c) 1998-2016 Zend Technologies
    with Zend OPcache v7.1.0-5+0~20161222133327.13+jessie~1.gbp46a191, Copyright (c) 1999-2016, by Zend Technologies
    with Xdebug v2.5.0, Copyright (c) 2002-2016, by Derick Rethans

I'm not sure if it's the most elegant way to install the required packages and how to avoid name conflicts.

Feedback is welcome.

vinzent · 2017-01-19T19:02:52Z

manifests/repo/debian.pp

+
+  if ($php::globals::php_version == '7.1') {
+    # Required packages for PHP 7.1 repository
+    package { 'apt-transport-https':


I do see that this package is required, but this could break others configuration if they defined the package in a profile.

the same for the other packages declared here.

I really would not do business of adding repos. and such packags. If doing the roles/profiles pattern it should be solved in the profile level. For me this module should not try to add repos. but thats my personal opinion. so many things fail if you would run such things in a restricted environment (no internet access, and the policy to use internal repos).

Right now I can't say what to do with these packages. :-/

I see conflict potential as well in the packages. So I'm also not sure.

If adding repos is not part of this module, then what's the purpose of this file debian.pp?

I'm not the original author of the module. I think it solves problems he or the users have. ;-)

Many modules solve this by having a "manage_repo" parameter in the init class.

You should probably also use ensure_packages to avoid conflicts with existing profiles.

never mind, I noticed that this is indeed gated by such a boolean. Should be fine then.

Please use stdlib::ensure_packages for this.

vinzent · 2017-01-19T19:04:07Z

manifests/repo/debian.pp

+    }
+
+    # Add PHP 7.1 key + repository
+    create_resources(::apt::key, { 'php::repo::debian-php71' => {


why using create_resources() here? why not just plain puppet code?

apt::key { 'php::repo::debian-php71': key => 'DF3D585DB8F0EB658690A554AC0E47584A7A714D', key_source => 'https://packages.sury.org/php/apt.gpg', }

True. Just copied from above. Can be plain puppet code.

vinzent · 2017-01-19T19:05:12Z

spec/spec_helper.rb

@@ -24,8 +24,8 @@
    puppetversion: Puppet.version,
    facterversion: Facter.version
  }
-  default_facts.merge!(YAML.load(File.read(File.expand_path('../default_facts.yml', __FILE__)))) if File.exist?(File.expand_path('../default_facts.yml', __FILE__))
-  default_facts.merge!(YAML.load(File.read(File.expand_path('../default_module_facts.yml', __FILE__)))) if File.exist?(File.expand_path('../default_module_facts.yml', __FILE__))
+  default_facts.merge!(YAML.safe_load(File.read(File.expand_path('../default_facts.yml', __FILE__)))) if File.exist?(File.expand_path('../default_facts.yml', __FILE__))


the safe_load rubocop was decided to be blacklisted. the load should be used.

vinzent · 2017-01-19T19:09:41Z

manifests/globals.pp

@@ -76,14 +76,14 @@
        }
      } else {
        case $globals_php_version {
-          /^7/: {
+          /^7\.[0-9]/: {


what problem is solved by changing the regex?

Being able to use:
$package_prefix = "php${globals_php_version}-"

Because the package prefix is php7.0- and not php7-
Before that change, the prefix was hardcoded to php7.0-.

smuggli · 2017-01-27T18:47:22Z

For all those who were waiting for a dotdeb release.

https://www.dotdeb.org/2017/01/27/php-7-1-dotdeb/

jyaworski · 2017-01-27T19:06:25Z

.rubocop.yml

@@ -302,7 +302,7 @@ Style/EmptyLiteral:
 Metrics/LineLength:
  Enabled: False

-Style/MethodCallParentheses:
+Style/MethodCallWithoutArgsParentheses:


Will modulesync overwrite this? @bastelfreak

@jyaworski no, its already in modulesync_config - https://github.com/voxpupuli/modulesync_config/blob/master/moduleroot/.rubocop.yml#L310

ffrank · 2017-10-03T00:15:56Z

Hi @fstr is this still on your plate? Do you need anything from our end?

jonhattan · 2018-02-03T09:04:17Z

This needs a reroll. The fix for package_prefix was already committed in #329

use ensure_packages dont wrap apt::key in a useless create_resources

This version automatically installs the apt-transport-https package, which is needed for https mirrors. By using this module in at least this version, we don't need to manage the package on our own.

this is the lowest supported version for ensure_packages

bastelfreak · 2018-02-03T10:13:19Z

I rebased the branch and implemented the changes @Vincent-- mentioned.

this is a private class, it won't get called directly, only from within the main class. It references variables from the main class, so we need to include it (or move the tests into the testfile for the main class)

jonhattan · 2018-02-03T17:22:55Z

Sorry I don't think this PR is the proper solution. At the end we want dotdeb for wheezy (php 5.6) and sury for jessie and stretch (5.6, 7.0, 7.1 and 7.2 as of now). I'll try to propose a PR in the coming days.

c33s · 2018-02-16T19:23:33Z

@jonhattan you are right, we want dotdeb for wheezy and sury for jessie and strech. ping me if you start working on a PR

this PR actually break things. it

uses the old apt:key syntax
ensure different packages as it require afterwards
uses the old facts syntax
hardcodes the repo key id
cannot be disabled (something like $sury = false)

EpiqSty · 2018-07-04T14:17:47Z

Hi,

First of all thanks for great work ;) We used that module for a while, but now we have tried to install PHP 7.1 on Debian 9 , and faced with those errors:

Error: Execution of '/usr/bin/apt-get -q -y -o DPkg::Options::=--force-confold install php7.1-cli' returned 100: Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package php7.1-cli
E: Couldn't find any package by glob 'php7.1-cli'
E: Couldn't find any package by regex 'php7.1-cli'
Error: /Stage[main]/Php::Packages/Package[php7.1-cli]/ensure: change from 'purged' to 'present' failed: Execution of '/usr/bin/apt-get -q -y -o DPkg::Options::=--force-confold install php7.1-cli' returned 100: Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package php7.1-cli
E: Couldn't find any package by glob 'php7.1-cli'
E: Couldn't find any package by regex 'php7.1-cli'
Error: Execution of '/usr/bin/apt-get -q -y -o DPkg::Options::=--force-confold install php7.1-common' returned 100: Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package php7.1-common
E: Couldn't find any package by glob 'php7.1-common'
E: Couldn't find any package by regex 'php7.1-common'
Error: /Stage[main]/Php::Packages/Package[php7.1-common]/ensure: change from 'purged' to 'present' failed: Execution of '/usr/bin/apt-get -q -y -o DPkg::Options::=--force-confold install php7.1-common' returned 100: Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package php7.1-common
E: Couldn't find any package by glob 'php7.1-common'
E: Couldn't find any package by regex 'php7.1-common'

Does it mean, that such combination (PHP7.1 & Debian 9) still not supported by module, or we have to add some custom configuration?

For now we just defined desired version this way:

php::globals::php_version: '7.1'

is it enough or not?

vinzent reviewed Jan 19, 2017

View reviewed changes

jyaworski reviewed Jan 27, 2017

View reviewed changes

Florian Strübe and others added 3 commits February 3, 2018 11:12

Add PHP 7.1 support on Debian

a2982b5

use ensure_packages dont wrap apt::key in a useless create_resources

require at least puppetlabs/apt 4.4.0

096f47b

This version automatically installs the apt-transport-https package, which is needed for https mirrors. By using this module in at least this version, we don't need to manage the package on our own.

require at least puppetlabs/stdlib 4.16.0

10b21da

this is the lowest supported version for ensure_packages

bastelfreak force-pushed the deb-php71 branch from d8ca3fe to d063e8d Compare February 3, 2018 10:12

add missing pre_condition for repo unit tests

3c82ae4

this is a private class, it won't get called directly, only from within the main class. It references variables from the main class, so we need to include it (or move the tests into the testfile for the main class)

bastelfreak force-pushed the deb-php71 branch from d063e8d to 3c82ae4 Compare February 3, 2018 10:14

bastelfreak added the enhancement New feature or request label Feb 3, 2018

bastelfreak merged commit f3c6e4a into voxpupuli:master Feb 3, 2018

jonhattan mentioned this pull request Feb 3, 2018

Use https://deb.sury.org/ as opposed to Dotdeb #403

Open

c33s added a commit to c33s/puppet-php that referenced this pull request Feb 16, 2018

removed php7.1 specific code (voxpupuli#293)

db577d2

c33s mentioned this pull request Feb 24, 2018

Fix wrongly named parameters for apt::source/key #420

Merged

SimonHoenscheid mentioned this pull request May 6, 2018

Debian Sury support, remove dotdeb support, add multiversion support for Sury #448

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHP 7.1 support on Debian #293

Add PHP 7.1 support on Debian #293

fstr commented Jan 16, 2017

vinzent Jan 19, 2017

fstr Jan 19, 2017

vinzent Jan 19, 2017

oranenj Jan 19, 2017

oranenj Jan 19, 2017

sts Mar 9, 2017

vinzent Jan 19, 2017

fstr Jan 19, 2017

vinzent Jan 19, 2017

vinzent Jan 19, 2017

fstr Jan 19, 2017 •

edited

Loading

smuggli commented Jan 27, 2017

jyaworski Jan 27, 2017

vinzent Jan 27, 2017

ffrank commented Oct 3, 2017

jonhattan commented Feb 3, 2018

bastelfreak commented Feb 3, 2018

jonhattan commented Feb 3, 2018

c33s commented Feb 16, 2018

EpiqSty commented Jul 4, 2018 •

edited

Loading

Add PHP 7.1 support on Debian #293

Add PHP 7.1 support on Debian #293

Conversation

fstr commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fstr Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

smuggli commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffrank commented Oct 3, 2017

jonhattan commented Feb 3, 2018

bastelfreak commented Feb 3, 2018

jonhattan commented Feb 3, 2018

c33s commented Feb 16, 2018

EpiqSty commented Jul 4, 2018 • edited Loading

fstr Jan 19, 2017 •

edited

Loading

EpiqSty commented Jul 4, 2018 •

edited

Loading