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

[Feature]: Add docker setup and an updated README for people who want to contribute #260

Merged
merged 6 commits into from
Jan 28, 2024

Conversation

sven-ahrens
Copy link
Contributor

Feature: Docker Setup for Testing

Motivation

I recently opened an issue (#259) and want to contribute to the project to help improve it. However, as a newcomer to Symfony and this specific project, I faced challenges getting started.

Firstly, I couldn't find a README or any documentation to guide me through the project setup. Additionally, running tests or performing other tasks seemed reliant on having PHP, Composer, MySQL, and other services installed locally, which is not an ideal practice.

To address this, I've created this branch to enhance the onboarding process for new contributors like myself.

How to Test

  1. Clone this branch.
  2. Run: docker-compose up -d to install the necessary services.
  3. Enter the container: docker-compose exec php-fpm bash.
  4. Install dependencies: composer install.
  5. Run tests and ensure everything works as expected: /vendor/bin/phpunit.

@sven-ahrens
Copy link
Contributor Author

Hm, actually in retrospect, I'm not sure if it's smart to ship a complete docker setup but I think the README would be a smart idea with maybe a reference to such docker setup or alternative and which services the project needs in order to get started like php, composer, sqlite etc.

doc/contributing.md Outdated Show resolved Hide resolved
doc/contributing.md Outdated Show resolved Hide resolved
doc/contributing.md Outdated Show resolved Hide resolved
doc/contributing.md Outdated Show resolved Hide resolved
doc/contributing.md Outdated Show resolved Hide resolved
@alexislefebvre
Copy link
Collaborator

Thanks for the PR.

Changing the configuration for the database connections break the CI since the hostnames don't exist on GitHub Actions.

Ideally, we would have a configuration that work in GitHub actions and on local docker-compose stack.

It looks like there is no way to start a database and reach it through the mysql or postgresql, even if that's the name of the service: https://github.com/liip/LiipTestFixturesBundle/blob/2.x/.github/workflows/tests.yml#L44

Maybe the easiest solution if to pass some variables like MYSQL_HOST=mysql to PHP in docker-compose.yml, and use something like this in the YAML file:

doctrine:
    dbal:
        driver: pdo_mysql
        host: '%env(default:127.0.0.1:file:MYSQL_HOST)%'

It should fallback to the current configuration when working without docker-compose.

@alexislefebvre
Copy link
Collaborator

We can try to modify .github/workflows/tests.yml in order to add aliases for the mariadb and postgres hosts so that GitHub Actions will be able to connect to the databases: https://stackoverflow.com/questions/66980682/is-it-possible-to-define-host-mappings-in-github-actions/66982842#66982842

MYSQL_ALLOW_EMPTY_PASSWORD should probably be removed and the root password should be provided in order to correspond to the YAML settings.

The file tests/AppConfigMysqlUrl/config.yml will have to be updated too.

@alexislefebvre
Copy link
Collaborator

alexislefebvre commented Jan 28, 2024

I made some small adjustments and now tests are passing on local environment through docker-compose and on GitHub Actions too.

For some reason, sudo apt install -y -q mariadb-client crashes on one job, it may be a temporary error. It worked 2 weeks ago: #256

Let's merge it as is.

Thanks for your contribution!


Update: apt update was missing, see:

@alexislefebvre alexislefebvre merged commit d355398 into liip:2.x Jan 28, 2024
9 of 10 checks passed
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.

2 participants