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.

Sign up for GitHub

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

Jump to bottom

update nginx::package to select the package class by $::osfamily #99

Merged
merged 1 commit into from
Aug 22, 2013
Merged

update nginx::package to select the package class by $::osfamily #99

merged 1 commit into from
Aug 22, 2013

Conversation

jhoblitt
Copy link
Member

There were some bugs in the existing $::operatingsystem based approach.

  • amazon was it's own package set when it's properly part of $::osfamily ==
    'redhat' (only in recent factors)
  • gentoo was improperly part of the amazon package set; this patch remove
    support for gentoo but it was broken anyways

modifications to nginx::package::redhat were made as well

  • it was trying to recover from $::lsbmajdistrelease not being defined -- this
    was removed.
  • the url to the nginx repo was including $::operatingsystem in it but
    nginx.org only has package dirs for 'rhel' & 'centos'; the usage of 'rhel' has
    been hardcoded.

I have only tested this patch on Scientific Linux 6.4, it likely needs additional platform testing before being merged.

@jhoblitt
Copy link
Member Author

amazon is only part of $::osfamily == 'redhat' in newer versions of factor. It would be easy to add a check for $::operationsystem == facter in the default block for case $::osfamily if that's deemed important.

@jhoblitt
Copy link
Member Author

jhoblitt commented Aug 3, 2013

Are there any comments on this PR? I need this patch to fix installation on Scientific Linux.

@jfryman
Copy link
Contributor

jfryman commented Aug 3, 2013

No, sorry. I have been off-site at GitHub Summit this week, so I have not been able to actively review/merge PRs in.

I would prefer that you add the $::operatingsystem check in the default block before I merge this. I haven't explicitly deprecated Facter 1.6.1 and earlier (this was introduced in 1.6.2), so I don't want to break anyone. However, if you could add a warning() block that says to upgrade, that would be 👍.

Once you make that change, I'll get this merged in today and uploaded.

@jhoblitt
Copy link
Member Author

jhoblitt commented Aug 3, 2013

I repushed too soon. I've found some issues in testing on real platforms that need to be resolved.

@jhoblitt
Copy link
Member Author

jhoblitt commented Aug 3, 2013

I've made the requested change and some additional fine tuning that was required after manually testing on Scientific 6.4, Fedora 18, and Amazon 2013. Note that since the first version of this PR the tests for Amazon and Fedora have been broken out of the shared_example for redhat. Feed back on the tests would be appreciated.

@jhoblitt
Copy link
Member Author

I've rebased the PR on the current master as a conflict had been merged since I opened this PR.

@jhoblitt
Copy link
Member Author

Is this PR in a merged state or are changes needed?

update nginx::package to select the package class by $::osfamily
8a1981f 
There were some bugs in the existing $::operatingsystem based approach.

* amazon was it's own package set when it's properly part of $::osfamily ==
'redhat' as of facter >= 1.7.2

* gentoo was improperly part of the amazon package set; this patch removes
support for gentoo but it was broken anyways

modifications to nginx::package::redhat were made as well

* it no longer tries to setup the nginx.org yumrepo for fedora as no packages
for fedora are currently provided

* amazon release numbers are inconsistent with EL.  Unknown
$::lsbmajdistrelease values are now mapped to 6 so it's no longer nessicary to
test for $::lsbmajdistrelease being undefined.  This logic will need to be
reworked after RHEL7.x is released.

* the url to the nginx repo was including $::operatingsystem in it but
nginx.org only has package dirs for 'rhel' & 'centos' which are presently
identical; the usage of the 'rhel' dir has been hardcoded.  This fixes broken
yum repo setup for all $::osfamily == 'redhat' platforms other than redhat and
centos.
@jfryman
Copy link
Contributor

jfryman commented Aug 22, 2013

Awesome. Thanks so much!

jfryman pushed a commit that referenced this pull request Aug 22, 2013
Merge pull request #99 from jhoblitt/select_package_by_osfamily
41591a3 
update nginx::package to select the package class by $::osfamily
@jfryman jfryman merged commit 41591a3 into voxpupuli:master Aug 22, 2013
@jhoblitt jhoblitt mentioned this pull request Aug 28, 2013
Merged
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Oct 23, 2017
Merge pull request voxpupuli#99 from jhoblitt/select_package_by_osfamily
fd97a44 
update nginx::package to select the package class by $::osfamily
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhoblitt @jfryman