Skip to content
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

Merged
merged 1 commit into from
Jul 27, 2018

Conversation

tphoney
Copy link

@tphoney tphoney commented Jul 27, 2018

No description provided.

Copy link
Collaborator

@ekohl ekohl left a 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.

@@ -32,6 +32,10 @@
if ! $location {
fail('cannot create a source entry without specifying a location')
}
$method = split($location, '[:\/]+')[0]
Copy link
Collaborator

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://

Copy link
Author

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.

Copy link
Author

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 👍

Copy link
Collaborator

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')
}

Copy link
Author

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')
}

Copy link
Collaborator

@ekohl ekohl left a 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.

@tphoney
Copy link
Author

tphoney commented Jul 27, 2018

thanks for the help / feedback @ekohl 🥇

@pmcmaw pmcmaw merged commit a8bc3a0 into puppetlabs:master Jul 27, 2018
@tphoney tphoney added the bugfix label Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants