-
-
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
Do not use a a hardcoded default for php::repo::debian #430
Conversation
manifests/repo/debian.pp
Outdated
::apt::source { 'dotdeb-wheezy': | ||
location => $location, | ||
::apt::source { 'php-wheezy': | ||
location => 'http://packages.dotdeb.org', |
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.
we can use $location here, right?
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 is the question, if we want this module to auto manage the repos, we have to add hardcoded locations. like
- if
stretch
andphp version
=7.1
or5.6
auto use sury - if
wheezy
... use dotdeb or sury (or sury for all, we have to build a matrix what repo should automatically be used
some kind of if location is undef -> use auto mode, else do not use any hardcoded repo
Hi @jkroepke, thanks for this bugfix. Can you add unit tests or an acceptance test that verifies it's now working correctly? |
All those repos and conditions are quite confusing. Can you please add tests for this? |
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 we have to discuss the direction of this module for debian a little bit. if not the next PR will surely make changes here again
manifests/repo/debian.pp
Outdated
$dotdeb = true, | ||
$sury = true, | ||
Boolean $dotdeb = false, | ||
Boolean $sury = false, |
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.
Bc break. both bool flags should stay on true or if the sury
parameter has not be released, we can change sury to false as default
manifests/repo/debian.pp
Outdated
@@ -47,7 +47,7 @@ | |||
source => $key['source'], | |||
}}) | |||
|
|||
::apt::source { "source_php_${release}": | |||
::apt::source { 'php-upstream': |
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.
@bastelfreak we should make this name configureable. i personally prefer to name my repo files according the upstream name dotdeb
or sury
and not according the content php
. both provide php but also other things. especially dotdeb provides a lot of other stuff
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 agree, but thats out of the scope here.
manifests/repo/debian.pp
Outdated
::apt::source { 'dotdeb-wheezy': | ||
location => $location, | ||
::apt::source { 'php-wheezy': | ||
location => 'http://packages.dotdeb.org', |
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 is the question, if we want this module to auto manage the repos, we have to add hardcoded locations. like
- if
stretch
andphp version
=7.1
or5.6
auto use sury - if
wheezy
... use dotdeb or sury (or sury for all, we have to build a matrix what repo should automatically be used
some kind of if location is undef -> use auto mode, else do not use any hardcoded repo
manifests/repo/debian.pp
Outdated
$repos = 'all', | ||
$include_src = false, | ||
$key = { | ||
String $location = 'http://packages.dotdeb.org', |
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.
if we use the auto stuff i described below, we also should set the default location and key hash to undef
Okay. The point of this PR is to support current debian version without changing the behavior old debian 7. Since I don't have any knowledge about debian repos (it doesn't looks like an charm), optimization should be done in a follow-up PR. |
Good enough for most cases. I removed some special handing. This class should be easier to maintain now. |
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.
in general i think we have to fix the debian tests before we refactor something. i would like to use the hiera module data for all the hardcoded stuff
}}) | ||
|
||
::apt::source { "source_php_${release}": | ||
apt::source { 'php': |
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.
should be named sury
and should be configurable
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 don't think so. The sury location is just daemon and can be replaced by any other location. I look around other voxpopuli modules. nginx just use nginx, collectd used ppa_collect. Use the software as apt::source
title should be good enough in my eyes like other modules do it.
'source' => 'http://www.dotdeb.org/dotdeb.gpg', | ||
}, | ||
$dotdeb = true, | ||
$sury = true, |
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.
we should introduce a enum parameter like repo_management
auto|native|sury|(dotdeb)
so the user can decide how the repo management behaves. it should be possible to set a custom repo and prevent the auto added repo from below
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.
Parameters like location
, key
are configurable via hiera. With this PR all standard deb repo like sury are covered by this change. I do not see a point for sury special handing.
} | ||
} | ||
|
||
if ($sury and $php::globals::php_version == '7.1') { |
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 would prefer keeping the if
with the repo_management
parameter. we also could keep the dotdeb repo data in here or better move the hard coded classes to module-hiera-data
}, | ||
require => [ | ||
Apt::Key['php::repo::debian-php71'], | ||
Package['apt-transport-https', 'lsb-release', 'ca-certificates'] |
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.
why can we drop this?
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.
apt-transport-https is handed by apt well.
https://github.com/puppetlabs/puppetlabs-apt/blob/master/manifests/source.pp#L41
lsb-release
and ca-certificates
are core packages, its out of scope here. For example apt recommends ca-certificates, you don't need define it here.
|
||
if ($sury and $php::globals::php_version == '7.1') { | ||
# Required packages for PHP 7.1 repository | ||
ensure_packages(['lsb-release', 'ca-certificates'], {'ensure' => 'present'}) |
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.
why can we drop this?
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.
See above.
@jkroepke can we chat somewhere? i already started with the refactoring plans before. mainly i would need help with the broken tests. |
Sure. Does voxpopuli already discord? Or just IRC? |
slack also ok? (online right now) |
What's the status of this now (other than the merge conflict)? |
On Debian 8 and 9 systems, the dotdeb debian 7 repo will be used by default. This shouldn't be.
Some breaking changes here since all location special handling removed for now.