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

Debian support #59

Merged
merged 12 commits into from
Aug 9, 2016
Merged

Debian support #59

merged 12 commits into from
Aug 9, 2016

Conversation

NITEMAN
Copy link
Contributor

@NITEMAN NITEMAN commented Aug 5, 2016

This PR adds:

  • Debian family compatibility (See params.pp)
  • Minor typo fix in install .pp
  • Repo support for Debian 8&9 (Jessie and Stretch)

I've borrowed some work from another fork.

Please let me know if anything more is needed in order to ofcially support Debian (I wish I know a little rspec to add some basic tests).

Tested on Debian 8

@@ -33,6 +33,14 @@
version => $version,
}
}
'Debian': {
if ! defined( '::apt' ) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should include the apt class here instead of the !defined

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 fear it wouldn't parse if there's no apt module available, but I'll proceed as you prefer

Copy link
Member

Choose a reason for hiding this comment

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

yep it will fail. I wrote a comment about adding the module to the metadata.json and fixtures as well. This should ensure that the module is available.


There was a longer discussion about soft dependencies and hard dependencies, we decided for hard deps. So we should include the module here + in metadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll change it

@bastelfreak
Copy link
Member

Hi @NITEMAN, thanks for the PR!
two things:

  • Can you please add the apt module to metadata.json and to .fixtures.yml?
  • please verify the email used in the first two commits, github is unable to recognize it

@NITEMAN
Copy link
Contributor Author

NITEMAN commented Aug 8, 2016

Hi @bastelfreak :

  • Added apt to fixtures. I don't think its a dependency (think of pure RedHat environments), so I'll left it outside metadata.json
  • That 2 commits came from another fork, I can squash everything... but I preferred to give credit to the original author

@NITEMAN
Copy link
Contributor Author

NITEMAN commented Aug 8, 2016

@bastelfreak everithing should be right now, let me know if anything more is needed

@bastelfreak
Copy link
Member

There are a few broken tests now, can you look into that?

@NITEMAN
Copy link
Contributor Author

NITEMAN commented Aug 8, 2016

I'll try my best, but I'm a noob in terms of rspec

@bastelfreak
Copy link
Member

if you've any questions feel free to join our IRC channel #voxpupuli on freenode. We should be able to solve the issues together.

dgibson and others added 10 commits August 8, 2016 18:28
@NITEMAN
Copy link
Contributor Author

NITEMAN commented Aug 8, 2016

@bastelfreak I think everything is ok right now, but please review carefully rspec files (especially brand new spec/classes/repo_apt_spec.rb)

Thank you very much for your support and thanks to @DarkGigabyte for him support ;)

PS: I have doubts on style offences highlighted by travis :(

end
it 'installs' do
should contain_apt__source('glusterfs-LATEST').with(
:repos => 'main',

Choose a reason for hiding this comment

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

The expected syntax for this is repos: 'main',. Same change is needed above starting at line 18.

@bastelfreak
Copy link
Member

thanks for the great work, I will review that tomorrow. Please ping me on IRC if I forget.

@QueerCodingGirl
Copy link
Contributor

A big thank you for @NITEMAN and @bastelfreak for the great work!


# basic sanity check
if $version == 'LATEST' {
$repo_ver = $version
Copy link
Member

Choose a reason for hiding this comment

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

should we use $_version instead of $repo_ver? We already use $_version in the other class, so it woul be consistent naming scheme.

@bastelfreak
Copy link
Member

Looks really good so far, I just made a little comment. Please take a look and then I'm happy to merge.

@bastelfreak
Copy link
Member

nevermind, I'm fine with the different name because it covers a different scope.

@bastelfreak bastelfreak merged commit c8fd443 into voxpupuli:master Aug 9, 2016
cegeka-jenkins pushed a commit to cegeka/puppet-gluster that referenced this pull request Jul 20, 2018
* Add Support for Debina-based distros

* Update README to include Debian support

* remove repository mess from Ubuntu PPA & Raspbian

remove ubuntu support for now (it'll be easier to add the test only for debian)

remove leftover comment

* fix variable name

* fix travis strict complains

fixing tests, take 1

fixing tests, take 2

fixing tests, take 3

fixing tests, take 4

fixing tests, take 5

* add apt to .fixtures

* remove apt guard

fix typo

* add puppetlabs/apt hard depencency

* add test for repo::apt

* change priority behaviour on apt::repo

* fix style offences, take 1

* fix style offences, take 2
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.

4 participants