-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
WIP: Feature/simon hoenscheid 603 restructure repository management #605
Conversation
|
||
if (($manage_internal_repo or $manage_external_repo) and (($php_version in $external_repo_supported_php_versions) or ($php_version in $os_supported_php_versions))) { | ||
case $facts['os']['name'] { | ||
'Archlinux': { contain 'php::repo::archlinux' } |
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.
for arch and freebsd, should we just do nothing? since those don't have external repositories.
'Archlinux': { contain 'php::repo::archlinux' } | |
'Archlinux': { } |
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.
The classes are for generally managing repos, is there no need to connect a specific one, for a specific php version? Kind of the same problem exists for RHEL 8 style distros, there is the need to activate a specific app stream
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 will implement this
Co-authored-by: Tim Meusel <tim@bastelfreak.de>
# added for refactoring | ||
Boolean $manage_external_repo = false, | ||
Array $external_repo_supported_php_versions = [], | ||
Hash $external_repo_details = {}, |
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 maybe "custom repo detail" or "repo config" or "repo settings" or simply "repository" (is it real relevant if it is a external or internal repository? isn't it "here we have custom repository config" (i can be the default debian repo where i only change a key hash)
-
should be a custom type like
type Profile::Apt::Source = Struct[{
Optional[location] => Optional[String[1]],
Optional[comment] => String[1],
Optional[ensure] => Optional[Enum['file', 'present', 'absent']],
Optional[release] => Optional[String],
Optional[repos] => String[1],
Optional[include] => Optional[Struct[{
Optional[src] => Boolean,
Optional[deb] => Boolean,
}]],
Optional[key] => Optional[Struct[{
Optional[id] => String[1],
Optional[source] => String[1],
}]],
Optional[pin] => Optional[Struct[{
Optional[explanation] => String[1],
Optional[packages] => String[1],
Optional[origin] => String[1],
Optional[priority] => Integer,
}]],
Optional[architecture] => Optional[String],
Optional[allow_unsigned] => Boolean,
Optional[notify_update] => Boolean,
# Optional[comment] => String[1],
# location => 'https://packages.gitlab.com/runner/gitlab-runner/debian/',
# release => 'stretch',
# repos => 'main',
# pin => {
# explanation => 'Prefer GitLab provided packages over the Debian native ones',
# packages => 'gitlab-runner',
# origin => 'packages.gitlab.com',
# priority => 1001,},
# key => {
# 'id' => '1A4C919DB987D435939638B914219A96E15E78F4',
# 'source' => 'https://packages.gitlab.com/runner/gitlab-runner/gpgkey',
# },
# include => {
# 'src' => true,
# 'deb' => 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.
does this work for every repo type?
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.
"yes" we would have to write a type for each repo stucture we want to support.
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 will add this type to the module
# defaults: latest stable PHP Version available on php.net, default for OS from Hiera Data in module | ||
# | ||
class php::repo ( | ||
Boolean $manage_internal_repo = $php::manage_internal_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.
not sure if we need internal and external as definition. isn't it simply repository? why we need to differ?
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 my opinion yes, because there are operating systems where we need to manage internal and external repos (RHEL/CENTOS/Fedora) app channel/Remi and if I want to have different data without interpolation this seems to be the way to go
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.
don't know about centos & co, you manage the internal and the external repo for php?
is it relevant if it is external or internal? isn't it just multiple repositories
?
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.
Its been a while since I worked with Fedora based systems, it only affects RHEL8/CENTOS8 and Fedora IDK which version RHEL8 was branched from +. For me this seems important, because it will prevent users declaring internal and external php versions/sources parallel.
Dear @SimonHoenscheid, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs? |
I would now remove all old tests so we can rewrite the tests for rewritten classes. So the test status is clear |
Fixes #603