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

Add Debian Stretch support #17

Merged
merged 10 commits into from
Mar 12, 2019

Conversation

smortex
Copy link
Contributor

@smortex smortex commented Mar 9, 2019

Debian Stretch (9) ships with OpenSSL 1.1.0, while previous versions
where using OpenSSL 1.0.0. Elastic maintains two repository for
handling this situation.

More info:
https://www.elastic.co/guide/en/elasticsearch/client/curator/5.3/apt-repository.html

Debian versions prior to 8 are obsolete and not supported.
Future Debian versions are currently not supported, and adding support
should be straightforward when they are released.

smortex added 5 commits March 8, 2019 19:26
Debian Stretch (9) ships with OpenSSL 1.1.0, while previous versions
where using OpenSSL 1.0.0.  Elastic maintains two repository for
handling this situation.

More info:
https://www.elastic.co/guide/en/elasticsearch/client/curator/5.3/apt-repository.html

Debian versions prior to 8 are obsolete and not supported.
Future Debian versions are currently not supported, and adding support
should be straightforward when they are released.
No version of Ubuntu seems to currently ships OpenSSL 1.1.0.
It's extensively used in Travis-CI configuration but has no effect.
@smortex smortex force-pushed the debian-stretch-support branch from 1a39566 to e9ba0bb Compare March 9, 2019 06:07
smortex added 2 commits March 8, 2019 20:08
Puppet 4.x has been EOL for a few month.  Test against puppet 5.x and
6.x.
* Add 18.04 which is part of the test suite and was missing;
* Remove 10.04 which has reached EOL.
@smortex smortex force-pushed the debian-stretch-support branch from e9ba0bb to fbfbf14 Compare March 9, 2019 06:09
@smortex
Copy link
Contributor Author

smortex commented Mar 9, 2019

After a few trial & errors, this seems to be building fine on Travis-CI \o/

Ready for review!

This was referenced Mar 9, 2019
Copy link
Owner

@mvisonneau mvisonneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution guys, awesome work. Few things I would like to get polished but otherwise LGTM 👍

manifests/repository/apt.pp Show resolved Hide resolved
$repo_suffix = $debian_major_version ? {
# Debian releases
'8' => '',
'9' => '9',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably easier :

$repo_suffix = $::os['release']['major'] ? {
  '9'         => '9',
  default => '',
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to do that way because Debian 10 will surely not work with the "debian" repo. It might work with the "debian9" repo, but maybe not 🤷‍♂️

It might be better to crash hard with unsupported systems, and add support for them accordingly when they are released?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, yes in that case let's leave the parameter $debian_major_version so that people can potentially use it with debian 10 before we patch the module 👍. However lets keep only 9 and default in the case statement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in b41fee3

Please note that I am not endorsing this since the module will successfully deploy a broken setup, and because of the way the module works, the end-user will not be aware of the breakage (until he discovers that curator is non-functional because he ran out of disk space).

For the record, here is what the problem looks like when you hit it: elastic/curator#1241

Copy link
Owner

@mvisonneau mvisonneau Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you prefer I'm happy to get what you used to have but make it fail on default then. Otherwise we're just going to get an undef value. Might also need to get a conditional around it to only execute the switch case on Debian base hosts though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe, when no default is present and no match is found, the selector fails (this is what I was relying on):

romain@zappy ~ % puppet apply -e '$x = $nobody_knows ? { 1 => "a", 2 => "b", } '                                                                            
Warning: Unknown variable: 'nobody_knows'. (line: 1, column: 6)
Error: Evaluation Error: No matching entry for selector parameter with value '' (line: 1, column: 6) on node zappy.home.sigabrt.org

You are right, a default with a better error message is probably the best!

metadata.json Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
.fixtures.yml Show resolved Hide resolved
.fixtures.yml Show resolved Hide resolved
@smortex smortex force-pushed the debian-stretch-support branch from 8556db2 to 1200aee Compare March 12, 2019 18:35
The maintainer prefers that the module does not fail hard when running
on an unsupported Linux distro.  Default to use the OpenSSL 1.0.0
repository expect from Debian 9 where OpenSSL 1.1.0 is installed.
@mvisonneau mvisonneau merged commit 96cf9f7 into mvisonneau:master Mar 12, 2019
@mvisonneau
Copy link
Owner

Thanks a lot @smortex ! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants