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 variables for Ubuntu 20.04 #215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bramd
Copy link

@bramd bramd commented Aug 20, 2020

PR progress checklist (to be filled in by reviewers)

  • Changes to documentation are appropriate (or tick if not required)
  • Changes to tests are appropriate (or tick if not required)
  • Reviews completed

What type of PR is this?

Primary type

  • [build] Changes related to the build system
  • [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
  • [ci] Changes to the continuous integration configuration
  • [feat] A new feature
  • [fix] A bug fix
  • [perf] A code change that improves performance
  • [refactor] A code change that neither fixes a bug nor adds a feature
  • [revert] A change used to revert a previous commit
  • [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

  • [docs] Documentation changes
  • [test] Adding missing or correcting existing tests

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

  • Updated the README (e.g. Available states).
  • Updated pillar.example.

Testing checklist

  • Included in Kitchen (i.e. under state_top).
  • Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
  • Updated the relevant test pillar.

Additional context

@pull-assistant
Copy link

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
Copy link
Member

myii commented Aug 25, 2020

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
Copy link

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
Copy link
Member

myii commented Oct 1, 2020

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
Copy link

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
Copy link
Member

myii commented Oct 1, 2020

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

@crosscodr
Copy link

Or maybe we should review and enhance #214.

Copy link
Member

@sticky-note sticky-note left a comment

Choose a reason for hiding this comment

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

LGTM

@sticky-note
Copy link
Member

@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
Copy link
Member

myii commented Feb 14, 2021

@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:

php-formula/.gitlab-ci.yml

Lines 136 to 149 in daa4c9e

debian-debian-10-master-py3: {extends: '.test_instance'}
debian-debian-9-master-py3: {extends: '.test_instance'}
ubuntu-ubuntu-2004-master-py3: {extends: '.test_instance'}
ubuntu-ubuntu-1804-master-py3: {extends: '.test_instance'}
ubuntu-ubuntu-1604-master-py3: {extends: '.test_instance'}
redhat-centos-8-master-py3: {extends: '.test_instance'}
redhat-centos-7-master-py3: {extends: '.test_instance'}
redhat-fedora-33-master-py3: {extends: '.test_instance'}
redhat-fedora-32-master-py3: {extends: '.test_instance'}
suse-opensuse-leap-152-master-py3: {extends: '.test_instance'}
suse-opensuse-tmbl-latest-master-py3: {extends: '.test_instance'}
redhat-amazonlinux-2-master-py3: {extends: '.test_instance'}
redhat-oraclelinux-8-master-py3: {extends: '.test_instance'}
redhat-oraclelinux-7-master-py3: {extends: '.test_instance'}

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

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