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

Repo-Remi for php7 on CentOS #51

Merged
merged 4 commits into from
Feb 5, 2018
Merged

Repo-Remi for php7 on CentOS #51

merged 4 commits into from
Feb 5, 2018

Conversation

seth-shaw-unlv
Copy link
Contributor

Addresses Drupal 8 upcoming requirement for PHP7 (see also this issue on the CLAW repo).

Based on a collection of pulls (see the related comment) on @DigitLib fork who did it a bit cleaner than I originally had.

@seth-shaw-unlv
Merge pull request #3 from Islandora-Devops/master
b65ef56 
Pull in Selinux and drupal user changes
@seth-shaw-unlv
Implement repo-remi for PHP7 in CentOS
54961c5 
Credit to @DigitLib who did this on their fork (https://github.com/DigitLib/claw-playbook/pulls) in a cleaner way than I initially did.
whikloj
whikloj reviewed Jan 29, 2018
View reviewed changes
inventory/vagrant/group_vars/webserver/php-RedHat.yml Outdated
@@ -0,0 +1,7 @@
---

php_date_timezone: "America/Toronto"
Copy link
Member

Choose a reason for hiding this comment

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

This variable is the same as in php-Debian.yml so I'd pull it out into a separate variable file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I would be fine simply removing it. The default from geerlingguy is Chicago, but I'm in Las Vegas, so setting a default for Toronto (although consistent w/ Islandora's origin) doesn't really make sense to me. Might as well set it to GMT.

Copy link
Member

Choose a reason for hiding this comment

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

@seth-shaw-unlv Good point @dannylamb @Natkeeran @jonathangreen what are your feelings on this, do we really need to set the datetime zone?

Copy link
Member

Choose a reason for hiding this comment

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

Sometimes things complain if it's not set. I'd just make it configurable with a default of GMT.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this at the CLAW call and the consensus is that as long as it is set (and it is set to Chicago as @seth-shaw-unlv said) then we can remove these

whikloj
whikloj reviewed Feb 5, 2018
View reviewed changes
requirements.yml
@@ -1,3 +1,8 @@
- src: geerlingguy.php-mysql
Copy link
Member

Choose a reason for hiding this comment

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

@seth-shaw-unlv should this have a version to pin it to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can. I will update the PR to pin the current version.

whikloj
whikloj approved these changes Feb 5, 2018
View reviewed changes
Copy link
Member

@whikloj whikloj left a comment

Choose a reason for hiding this comment

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

Tested in both Centos and Ubuntu and it builds and installs PHP 7.0.22. I think moving forward we need to get a build infrastructure to test this playbook. I have been guilty of building in Ubuntu and approving and that is causing more and more trouble

@whikloj whikloj merged commit d4990d3 into Islandora-Devops:master Feb 5, 2018
@seth-shaw-unlv seth-shaw-unlv deleted the remi-php branch February 5, 2018 17:22
@jonathangreen jonathangreen mentioned this pull request Feb 5, 2018
Closed
14 tasks
@whikloj
Copy link
Member

whikloj commented Feb 7, 2018

@seth-shaw-unlv looking at this one again, what loads the php-{{ ansible_os_family }}.yml file? Is that loaded from geerlingguy.php? I think my problem is that (and I can't explain why it did work before) the php-Debian.yml gets loaded each time and php-Redhat.yml gets ignored. So my centos/7 builds are failing on trying to load Ubuntu php packages.

@seth-shaw-unlv
Copy link
Contributor Author

I assume it is through geerlingguy (I'm not seeing it in ours). I think that piece, specifically, was taken from @DigitLib who might remember better.

@DigitLib
Copy link
Contributor

DigitLib commented Feb 7, 2018 •
edited
Loading

@whikloj and @seth-shaw-unlv it works when I test this soloution (it works a couple of times) . I tried now but it failed. Try to catch what is going on...
What vagrant version you have? My is 2.0.2

@DigitLib
Copy link
Contributor

DigitLib commented Feb 7, 2018 •
edited
Loading

@whikloj and @seth-shaw-unlv renamed php-Debian.yml and php-RedHat.yml to Debian.yml and RedHat.yml and it passed.
@whikloj Can you try to rename it and tell what is happened?
It is in geerlingguy.php vars define of php-files so I presume it is connected with a file name in our /webserver dir.

@seth-shaw-unlv
Copy link
Contributor Author

Well, it appears to have passed for me, too.

@DigitLib
Copy link
Contributor

DigitLib commented Feb 8, 2018

@seth-shaw-unlv Great! Now we wait will it pass @whikloj . It would be good if someone would try it on Ubuntu, so to PR if it work.

@seth-shaw-unlv
Copy link
Contributor Author

I tried it with both.

@DigitLib
Copy link
Contributor

DigitLib commented Feb 8, 2018

Great!!

@whikloj
Copy link
Member

whikloj commented Feb 8, 2018

Okay that does work 👍 , my only question is... is that how it is supposed to work? Meaning is geerlingguy expecting us to provide OS specific variable this way?

@DigitLib
Copy link
Contributor

DigitLib commented Feb 8, 2018

@whikloj I think so, because in his php role in ../tasks/main.yml he make this Variable setup.

  • name: Include OS-specific variables.
    include_vars: "{{ ansible_os_family }}.yml"

which is in ../var defined on that way. As you can see in that files he check packages etc...
I think it will recognize in our playbook this OS variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannylamb dannylamb dannylamb left review comments

@whikloj whikloj whikloj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seth-shaw-unlv @whikloj @DigitLib @dannylamb