-
Notifications
You must be signed in to change notification settings - Fork 462
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
(MODULES-7540) add apt-transport-https with https #775
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.
You've left out the $_transport_https_releases = [ 'wheezy', 'jessie', 'stretch', 'trusty', 'xenial' ]
check. In Debian Buster and Ubuntu Bionic it's a transitional package that's no longer needed.
manifests/source.pp
Outdated
@@ -32,6 +32,10 @@ | |||
if ! $location { | |||
fail('cannot create a source entry without specifying a location') | |||
} | |||
$method = split($location, '[:\/]+')[0] |
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 an odd regex because it'll also match :/:/
but I guess it works. You could also compare it to the Stdlib::Httpsurl
type to verify it starts with https://
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 looked at coding this approach. However because location is just a string, the comparison would not work. location can pretty much be anything.
IE the following will fail
if type(location) == Stdlib::HTTPSUrl {
ensure_packages('apt-transport-https')
}
Even changing the definition of location at the top will mean it will always be reported as a string.
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.
Re $_transport_https_releases = [ 'wheezy', 'jessie', 'stretch', 'trusty', 'xenial' ]
Shakes head, i dislike lists like this as they never get updated. However i missed the point that this is a finite list. With newer oses not needing the package.
Great catch @ekohl 👍
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.
Wouldn't if $location =~ Stdlib::Httpsurl
work?
The following does call fail:
$x = 'https://fsd'
if $x =~ Stdlib::Httpsurl {
fail('http only')
}
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.
It does work, apologies. I did find another bug though. This will not hit the fail
$x = 'HTTPS://fsd'
if $x =~ Stdlib::Httpsurl {
fail('http only')
}
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. The build failure looks like a transient failure.
thanks for the help / feedback @ekohl 🥇 |
No description provided.