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

Compatibility with Softwarecollections (SCL) #292

Closed
wants to merge 1 commit into from
Closed

Compatibility with Softwarecollections (SCL) #292

wants to merge 1 commit into from

Conversation

diLLec
Copy link
Contributor

@diLLec diLLec commented Jan 13, 2017

While using this module with RHEL/CentOS and SCL-PHP (https://www.softwarecollections.org/en), I came across some problems (mainly path and name specifics). Please review my changes and merge them to master.

Greetings,
Michael

@diLLec diLLec changed the title - expose pid_file of php::fpm::config Compatibility with Softwarecollections (SCL) Jan 13, 2017
@diLLec diLLec force-pushed the master branch 3 times, most recently from 1443ca0 to 10c76d0 Compare January 14, 2017 00:07
vinzent
vinzent requested changes Jan 19, 2017
View reviewed changes
Copy link
Contributor

@vinzent vinzent left a comment

Choose a reason for hiding this comment

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

thanks for your PR! I had a quick look.

the tests need to be fixed. if you're new to rspec testing: you can run bundle exec rake spec, bundle exec rake validate and bundle exec rake rubocop locally to test on your machine.

does it introduce breaking changes for existing users?

@vinzent vinzent added docs Improvements or additions to documentation enhancement New feature or request question labels Jan 19, 2017
@diLLec
Copy link
Contributor Author

diLLec commented Jan 23, 2017

ok, I'll look into it

@diLLec diLLec force-pushed the master branch 3 times, most recently from 1d5bd1c to ebb6ac7 Compare January 27, 2017 08:27
@diLLec
Copy link
Contributor Author

diLLec commented Jan 27, 2017

Hi @vinzent,

the tests are fine now. The changes do not introduce any breaking changes since the additional parameters are optional. Please verify and merge :)

Greetings,
diLLec

@vinzent
Copy link
Contributor

vinzent commented Jan 27, 2017

@diLLec could you rebase your commits to get a clean commit history?

  • including the title and text of the Github PR in the first commit
  • squashing all other commits to the first

howto: git fetch upstream && git rebase -i upstream/master, replace first pick with reword and all other pick with fixup. This should leave you with one commit for all - check with git log. and then git push --force

@diLLec
Copy link
Contributor Author

diLLec commented Jan 27, 2017

@vinzent thanks for the howto - commit is nice and clean now

@jsha09
Copy link

jsha09 commented Aug 2, 2017

Hi ... Any chance this change will get merged in soon?

@bastelfreak
Copy link
Member

@jsha09 I would love to, but we've a long list of conflicts here. This feature branch needs to be rebased. Feel free to do this and submit it as a new PR. We're happy to review and merge it.

@diLLec are you still interested in this PR? Can you please rebase it?

@diLLec diLLec force-pushed the master branch 10 times, most recently from 5dd31f6 to 135f921 Compare March 13, 2018 23:48
@diLLec diLLec force-pushed the master branch 4 times, most recently from faf0366 to d689f4b Compare March 14, 2018 08:08
@bastelfreak
Copy link
Member

bastelfreak commented Mar 14, 2018 •
edited
Loading

Thanks for rebasing. Can you take a look at the used email address in the commit? It isn't associated with your github account. Can you also take a look at the failing spec tests?

@bastelfreak bastelfreak added needs-work not ready to merge just yet tests-fail labels Mar 14, 2018
@diLLec
PR#292 Compatibility with Softwarecollections (SCL)
90f5f0a 
This adds RedHat SCL compatibility to this puppet module
by adding the rhscl_mode parameter to ::php::globals. By
specifying this parameter the module itself configures
path specifications on its own based on the php_version
string. Further the ::php::extensions define was extended
by the ability to prefix configs and stack up extensions
which are merged into one os package.
@diLLec
Copy link
Contributor Author

diLLec commented Mar 19, 2018

Hi,

the code is rebased now and tests are passing (only the debian install seams to be broken - issue that is not related to the changes of this PR).

Greetings,
Michael

@c33s c33s mentioned this pull request Mar 20, 2018
Closed
@oranenj oranenj mentioned this pull request May 10, 2018
Merged
@oranenj
Copy link
Contributor

oranenj commented May 10, 2018

I fixed the conflicts and did another PR, and @bastelfreak merged it, so this can be closed

Thanks to @diLLec for the work!

@oranenj oranenj closed this May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinzent vinzent vinzent requested changes

Assignees
No one assigned
Labels
docs Improvements or additions to documentation enhancement New feature or request needs-work not ready to merge just yet tests-fail
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@diLLec @vinzent @jsha09 @bastelfreak @oranenj