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

New structure for new Ubuntu PPA and other fixes #113

Merged
merged 30 commits into from
Dec 13, 2016
Merged

New structure for new Ubuntu PPA and other fixes #113

merged 30 commits into from
Dec 13, 2016

Conversation

sc250024
Copy link
Contributor

@sc250024 sc250024 commented Dec 2, 2016

Overview

READY FOR REVIEW

This is a major update to the php-formula that has been needed since the release of the updated ondrej/php PPA formula. When updating this formula, I attempted to keep much of the existing logic intact, while allowing for the new structure introduced by the PPA.

Issues Solved

Changes

  • Changed the map.jinja files to use the following decision logic centered around the PPA, and the specific version named in the Pillar file:
# If OS is Ubuntu (PPA only supports Ubuntu)
  # If the user wants to use the PPA
    # If the Ubuntu version is 16.04 or greater
      # Use >= 16.04 PPA map
    # Else
      # Use < 16.04 PPA map
    # EndIf
  # Else
    # If the Ubuntu version is 16.04 or greater
      # Use the >= 16.04 native OS package map
    # Else
      # Use the < 16.04 native OS package map
    # EndIf
  # EndIf
# Else
  # Use other OS package map
# EndIf
  • Updated suhosin states to compile from and use corresponding Git repositories (PHP 5.X or 7.X)
  • Updated mongo states to compile from PECL (needed build packages and corresponding phpenmod commands)
  • Updated all regular php states to include the main php.init.sls
  • Updated the php.ng.composer state to download from Composer website just as the php.composer state does
  • Removed php.ng.ffpmeg as it's an older module, and FFMPEG support can be obtained by using a Composer dependency (https://github.com/PHP-FFMpeg/PHP-FFMpeg)
  • Removed php.ng.twig since can be obtained by using a Composer dependency (http://twig.sensiolabs.org/doc/intro.html)
  • Updated php/ng/installed.jinja to account for the newer PPA strucutre and naming

Help Needed

  • How to handle states like gearman, gmp, net4 which, without a PPA, has an installation candidate in Ubuntu Trusty and below, but not in Ubuntu Xenial? And also not in CentOS?
  • Does the structure in the newer map.jinja files make sense? Are there any problems that I didn't forsee?
  • Checking package definitions for each distro to make sure they are accurate (especially APC vs. APCu)
  • Testing on Ubuntu 12.04 LTS, Arch Linux, and SUSE distributions
  • In the php/ng/installed.jinja I attempted to fix the problem that occurs when using the PPA. Sometimes the target /etc/alternatives/php get's changed on Ubuntu 16.04 to a version you didn't specify in the pillar data. I figured having a state that checked whether the current target used by /etc/alternatives/php matches what the user wants in their pillar. Is there a better way to go about this? Does anyone else encounter this problem? What about for HHVM installs where the /etc/alternatives/php should actually point to hhvm ?

@sc250024 sc250024 changed the title New structure for new Ubuntu PPA and other fixed New structure for new Ubuntu PPA and other fixes Dec 2, 2016
@aboe76
Copy link
Member

aboe76 commented Dec 3, 2016

@sc250024 , let me know when I can test this for you,
I have a arch linux vm ready with php 7 states,

@sc250024
Copy link
Contributor Author

sc250024 commented Dec 3, 2016

@aboe76 Thanks for the help! I'll let you know, I'm making some changes right now for Debian Jessie vs. Wheezy. I'll ping you again when I do ArchLinux next.

@sc250024
Copy link
Contributor Author

sc250024 commented Dec 3, 2016

@aboe76 Go ahead with the Arch Linux testing

@aboe76
Copy link
Member

aboe76 commented Dec 3, 2016 via email

@aboe76
Copy link
Member

aboe76 commented Dec 4, 2016

@sc250024 first test on Archlinux, this was with php:ng states.

  • no changes between old formula and your new changes so that's good !

@sc250024
Copy link
Contributor Author

sc250024 commented Dec 4, 2016

@aboe76 What do you recommend for cases where one distro has an installation candidate, but another does not? For example, I did a quick test on ArchLinux, and the default repos don't have a candidate for php-pear. Is it okay to have the state just fail if someone on ArchLinux called php.pear or php.ng.pear ?

@aboe76
Copy link
Member

aboe76 commented Dec 4, 2016

@sc250024 php-pear on archlinux is provided with an AUR package, which must be installed by hand by the user. I would recommend leaving the old behavior, don't include the php-pear package in the archlinux map.jinja so the formula won't install on archlinux. If an Arch user wants to use php-pear he will be smart enough to install it himself.

@sc250024
Copy link
Contributor Author

sc250024 commented Dec 8, 2016

@gravyboat I've merged the mongo changes you made today. Would you mind taking a look at this PR as well please?

Copy link
Member

@aboe76 aboe76 left a comment

Choose a reason for hiding this comment

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

I have found no issues, with while testing this.
Tested with php.ng states,
on OS: archlinux, debian jessie, debian stretch.

@gravyboat
Copy link
Contributor

Yeah this looks good to me as well, going to merge. Thanks @sc250024!

@gravyboat gravyboat merged commit 1e3e303 into saltstack-formulas:master Dec 13, 2016
@aboe76
Copy link
Member

aboe76 commented Dec 13, 2016

@sc250024 I wanted to test this formula with a debian vm with php 7.0 from dotdeb,
and it failed, because I didn't want to add external_repo from ppa (because debian not ubuntu).

Could you take a look at the logical map.jinja order to see if you can spot a way that debian can be set with php version 7.0 and not add the ppa repo?

@sc250024 sc250024 deleted the feature/NewStructureForPPA branch December 16, 2016 04:30
@sc250024
Copy link
Contributor Author

@aboe76: currently working on another pull request because I noticed that as well. I will also integrate the Red hat Webtatic PHP repository as well. This way, use_external_repo will be generic.

@aboe76
Copy link
Member

aboe76 commented Dec 19, 2016 via email

@aboe76
Copy link
Member

aboe76 commented Dec 19, 2016

I was alsof looking dit a solution, I think the postgres-formula mapping might help.

@wwentland
Copy link
Contributor

This unfortunately breaks our deployment as the php.fpm state now includes php which causes the installation of libapache2-mod-php5 even though we would like to use php5-fpm on nginx.

The fact that we would like to use php5-fpm shouldn't come as a surprise when users include the php.fpm state and doing so really shouldn't install the php5 metapackage.

I believe that this is done here as php/init.sls conflates both the repository management and metapackage installation.

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