-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
allow php 7.2 #455
allow php 7.2 #455
Conversation
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.
look good at first pass ;)
@@ -77,6 +77,7 @@ | |||
} | |||
} else { | |||
case $globals_php_version { | |||
/^5\.6/, |
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.
where was this before?
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.
this statement is new. The package names for php5.6 from the sury repo take the form php5.6-* and not the form php5-* of the packge names from the debian repos. Therefore we also need this if we use 5.6 with sury repos. The test fails on debian 7. Debian 7 is not supported by sury. The reason might be that it is EOL now: https://wiki.debian.org/LTS. Therefore you might choose to drop support. If you choose to keep Debian 7 support we have to do some tests here and in repo/debian.pp and disable sury repos for debian 7.
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.
yupp, that's what travis is showing https://travis-ci.org/voxpupuli/puppet-php/jobs/391234923#L1338
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.
so what should We Do? Drop Debian 7 Support or more code?
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.
Everything that is Debian 7 specific can go away. We don't support it anymore. But we still have to support PHP 5.6.
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.
actually, we could drop PHP 5.6 now.
manifests/repo/debian.pp
Outdated
id => 'DF3D585DB8F0EB658690A554AC0E47584A7A714D', | ||
source => 'https://packages.sury.org/php/apt.gpg', | ||
} | ||
|
||
::apt::source { 'source_php_71': | ||
::apt::source { 'source_php_sury': |
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.
Is that the name of the created file in /etc/apt/sources.list.d/
? So people would end up with two files that contain the same repo?
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.
I think it's a good idea to remove the "7.1" mark in the repo name. Sury is stable and will be used too for future versions.
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.
I also think this is needed in the long run. Is it a good idea to define a ensure=>absent resource for the old source_php_71 resource to prevent duplicates?
Pull Request (PR) description
Sury supports php 7.2 for debian. As php 7.1 (and 7.0) will be discontinued end of this year it might be needed to migrate to 7.2. As 7.0 is the only supported Version in official Debian 9 repos. I think that this Module should support PHP 7.2 from sury.
This Pull Request (PR) fixes the following issues
n/a