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

Add filter for commonly bundled PHP packages #41

Merged
merged 6 commits into from
Aug 14, 2017
Merged

Add filter for commonly bundled PHP packages #41

merged 6 commits into from
Aug 14, 2017

Conversation

ganto
Copy link
Contributor

@ganto ganto commented Aug 11, 2017

The PHP packaging between distributions and releases might slightly be different. For an example where this is an issue see #40.

This PR adds php__included_packages which takes a list of PHP packages included in the php-common APT package. It further pre-defines a list of included packages for the most commonly used distribution releases.

This change shouldn't affect any existing roles. However, after merging it might be possible to list the full PHP requirements in a consumer role no matter if it's packaged in the common package or a separate APT package. E.g.

owncloud__required_php_packages:
  # Included in base install:
  # - 'ctype'
  # - 'dom'
  # - 'iconv'
  - 'gd'
  - 'json'

Can then be expressed as:

owncloud__required_php_packages:
  - 'ctype'
  - 'dom'
  - 'iconv'
  - 'gd'
  - 'json'

I'm not at all familiar with the PHP naming, but I somehow feel, that this PR is fuzzing the expression package which now can mean Debian package or PHP module(?). This is a bit unfortunate but clarifying this would mean a much larger change. What do you think?

@ganto
Copy link
Contributor Author

ganto commented Aug 14, 2017

Seems that PHP packages are also called packages, so the naming is not so wrong after all. 😄

@ypid maybe you also want to have a look at it?

@ganto
Copy link
Contributor Author

ganto commented Aug 14, 2017

I think I mostly got the packages right for the upstream APT packages, but still need to fix the Ondrej Sury repository packages. At least this seems to be the cause of debops-contrib/ansible-roundcube#24 (comment).

Copy link
Member

@drybjed drybjed left a comment

Choose a reason for hiding this comment

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

I've tested this PR on Debian Jessie with and without Sury repositories and on Debian Stretch. It seems to work correctly.

@ganto, let me know when it's ready to merge.

@ganto
Copy link
Contributor Author

ganto commented Aug 14, 2017

Ok, glad to hear.

Still trying to fix the issue with the Roundcube CI test mentioned above. Although my last commit might probably fix an issue I only encountered mentally, it doesn't make the Roundcube test happy yet. Somewhere is still a bug (or misunderstanding) hidden. Will let you know, once I figured it out.

@drybjed
Copy link
Member

drybjed commented Aug 14, 2017

@ganto OK.

BTW, since you are working on a role in debops-contrib, did you see my last mailing list post? I'm thinking about merging everything back together: https://lists.debops.org/pipermail/debops-users/2017-August/000078.html

ganto added a commit to ganto/ansible-roundcube that referenced this pull request Aug 14, 2017
@ganto
Copy link
Contributor Author

ganto commented Aug 14, 2017

Ok, you can merge now. 👍

I found the issue (forgot to list the 'xml' package).

I'm thinking about merging everything back together: https://lists.debops.org/pipermail/debops-users/2017-August/000078.html

Haven't seen before, but thanks for mentioning. I also had some thoughts that I will reply to your mail.

@drybjed
Copy link
Member

drybjed commented Aug 14, 2017

@ganto, actually, I'll do it since I'll need to do a new role release, otherwise it won't be available for the Roundcube role.

@drybjed drybjed merged commit 4325353 into debops:master Aug 14, 2017
@ypid
Copy link
Member

ypid commented Aug 14, 2017

Nice work guys! Thanks.

I'm thinking about merging everything back together: https://lists.debops.org/pipermail/debops-users/2017-August/000078.html

Seen that. But it was a bit TL;DR for yesterday. Lets see about today :)

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.

3 participants