search

saltstack-formulas / php-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
57 stars 231 forks source link

Add variables for Ubuntu 20.04 #215

Open bramd opened 4 years ago

bramd commented 4 years ago

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

The formula did not work out of the box on a fresh Ubuntu 20.04 system. Some PHP 7.2 paths and package names were hardcoded, where Ubuntu 20.04 repositories contain PHP 7.4.

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

pull-assistant[bot] commented 4 years ago
Score: 1.00

Best reviewed: commit by commit

Optimal code review plan

     Add variables for Ubuntu 20.04

Powered by Pull Assistant. Last update 70ea2e2 ... 70ea2e2. Read the comment docs.

myii commented 4 years ago

Thanks for this PR, @bramd -- hopefully @sticky-note will get a chance to review this soon. Generally looks fine (other than the fact the map.jinja is already way too unwieldy). We should really use our latest 20.04 image here as well to test these changes; I'll try to do that in the near future.

crosscodr commented 4 years ago

I had problems with this too. The commit looks good to me, although there might be better solutions for map.jinja. However, can this pull-request still be accepted?

myii commented 4 years ago

I had problems with this too. The commit looks good to me, although there might be better solutions for map.jinja. However, can this pull-request still be accepted?

@crosscodr Can you confirm that these changes work for you?

crosscodr commented 4 years ago

Almost, but a few packages are not available in the official ubuntu-sources: I searched for the dependency-packages (php- without specific php version) provided in this PR, that differ from the previous section for Ubuntu >= 18.04: php-xsl php-tidy php-opcache php-dba php-dev php-bz2 php-bcmath. (Those are explicitly pinned to php7.2 in the 18.04-section in map.jinja) However, some of those are not found in the ubuntu default sources: php-xsl, php-opcache and php-dba These should be changed to php7.4-xsl, php7.4-opcache and php7.4-dba.

I also noticed, that this part

                        'SQL': {
                            'sql.safe_mode': 'Off'
                        },

can be omitted, hence it is removed since php7.2 (https://www.php.net/manual/de/ini.core.php#ini.sql.safe-mode).

This are my findings so far. There may be other things to optimise, but these are the main problems I can see.

myii commented 4 years ago

Thanks @crosscodr. @bramd can you use the last comment to update this PR?

crosscodr commented 4 years ago

Or maybe we should review and enhance #214.

sticky-note commented 3 years ago

@Myii How can I pull this branch to make some rebase aand fixes before merging. I don't have right to write on @bramd repo

myii commented 3 years ago

@myii How can I pull this branch to make some rebase aand fixes before merging. I don't have right to write on @bramd repo

Apologies for the delay, @sticky-note -- the CI situation is still taking up a lot of my attention.

There are a number of ways to approach this but I don't want to write a whole essay here! The easiest suggestion is to simply open up another PR, using the same commit again (with your amendments). That way, the authoring credit still goes back to the original author.

Rebasing would be good because I just upgraded the CI matrix here to include the latest platforms including ubuntu-20.04:

https://github.com/saltstack-formulas/php-formula/blob/daa4c9ef43da8bbe45d5068c280dbd85cad17809/.gitlab-ci.yml#L136-L149

Any changes you make will have to be reflected in the relevant map.jinja verification files here: