[WIP] Use action services and setup-php in CI workflow

[erikgaal]

As discussed sometime on Discord I'm opening a PR to use Github Actions native services and shivammathur/setup-php. This way we don't have to maintain a Dockerfile for this project anymore and immediately allows us to test on PHP 8.1 (so #792 is redundant).

[stancl]

The Dockerfile is primarily for local development, and the CI setup uses it as well, to keep the dev & CI environments consistent.

That said, I'm open to changing these. I'm looking at the original Dockerfile and trying to find what was the main purpose of it.

Seems like:

• variable PHP version (it's fine to only have this in CI using shivammathur/setup-php)
• phpredis extension
• pcov for coverage — not sure if we're still using this. I think there are now better ways to generate coverage reports? I'm fine with moving to something else. But I think the original purpose was to make coverage reports from phpunit faster
• pgsql & other DB extensions, I assume these are in shivammathur/setup-php as well

If these can be replaced with shivammathur/setup-php, we can make the switch and get rid of the Dockerfile altogether.

Seems like someone has an issue with building it as well (#794), so moving to this setup is probably the right decision.

Locally, there'd only be a docker-compose.yml file for starting all of the services used by tests.

The only downside of removing the Dockerfile is that we'd have some demands for the local setup of people who clone this package. Namely, that they have phpredis set up. But with a good CI setup, it's probably irrelevant for minor contributions.

So, in this PR I'm wondering if:

1. the CI file could use docker-compose.yml, instead of defining the services manually
2. shivammathur/setup-php can replace all of the dependencies we have in our Dockerfile: phpredis, possibly pcov, and the DB extensions

[rawmarius]

This setup is nice and convenient (also great performance) but if you allow me I think it's a great advantage to support a docker or a sail setup for developers who want to keep their local installation "clean" (just not install anything locally). It would also help to test with more databases more easily, for example testing with mysql 8.

[stancl]

I think it's not that important because it's only relevant for people contributing to the package. And the system requirements are common enough. But ideally, there'd be a simple Dockerfile shipped, yeah.

[erikgaal]

Unfortunately there's no Sail support for package development, that would be cool to have.

As far as I can see there's no way to easily run setup-php in your own Dockerfile, so you can mirror the Github Action environment. I would suggest to update the Dockerfile to use the shivammathur/node image and enable the extensions that we require manually.

Would you like that to be a separate PR, so we can merge this? @stancl

[stancl]

@erikgaal Does setup-php support the PHP extensions we had in our original Dockerfile?

[erikgaal]

Yep, redis and pcov are enabled. :)

[stancl]

Cool, in that case we can simplify the Dockerfile to use node as you mentioned. And use setup-php in GitHub Actions since they're similar enough.

Ideally, I'd like to do this all in one PR.

Aside from the Dockerfile, it'd be great if we could reuse the docker-compose.yml in GH actions as I mentioned above. And as for PHP 8.1, I'd go with a matrix that runs:

• L6 @ 7.4
• L8 @ 8.0
• L9 @ 8.1

This way, all three PHP versions are covered, and the most relevant Laravel versions are covered.

[erikgaal]

Unfortunately, I don't really have time to finish this PR shortly. If someone wants to continue, feel free to do so!

[stancl]

Remaining tasks here:

• [x] 1. See if the Dockerfile can be rewritten from scratch to use shivammathur/node as mentioned here https://github.com/archtechx/tenancy/pull/793#issuecomment-1033577726. Note: I recently added more dependencies to the Dockerfile here https://github.com/archtechx/tenancy/commit/cc6d4fe0ddcec405f645fbd2d8e90ab71afb1bec#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557. I'm not 100% sure if we can migrate to node. It'd be ideal if we could, though.
• [x] 2. Verify that the variables used here https://github.com/archtechx/tenancy/pull/793#discussion_r805854785 will work even if we have a matrix of multiple PHP/Laravel versions. Either the variables should not be shared between different GH Actions environments, or the keys should be modified to include the versions in them
• [x] 3. Update the matrix to run the 3 jobs described here https://github.com/archtechx/tenancy/pull/793#issuecomment-1034026507

[abrardev99]

Updated

Note: I merged the 3.x branch into this PR to resolve the conflicts.

Dockerfile

I've rewritten the Dockerfile and used the node container as the base and enabled some extensions manually like redis and sqlsrv. node installed PHP versions from 5.6 to 8.1. To enable extensions, we had to use the PHP version with them like /etc/php/8.1/mods-available/redis.ini; otherwise, the build failed.

This image was published here https://hub.docker.com/repository/docker/abrardev/tenancy, and I'm pulling the image in CI.

CI

I've improved the CI to run to multiple Laravel and PHP versions. Also, added the sqlsrv service.

Todo (Done)

I think we still can improve the Dockerfile by spitting the PHP version extensions enabling part. Also, it Should accept the PHP_VERSION as the container argument.

Todo

Published Docker image under ArchTech docker hub account.

[codecov-commenter]

Codecov Report

Merging #793 (f5044d9) into master (a0192a9) will increase coverage by 0.99%. The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master     #793      +/-   ##
============================================
+ Coverage     86.38%   87.37%   +0.99%     
- Complexity      355      447      +92     
============================================
  Files            99      108       +9     
  Lines          1072     1220     +148     
============================================
+ Hits            926     1066     +140     
- Misses          146      154       +8     

Impacted Files	Coverage Δ
src/Commands/Install.php	93.75% <ø> (-1.25%)	:arrow_down:
src/Commands/TenantList.php	0.00% <0.00%> (ø)
src/Database/Concerns/TenantRun.php	100.00% <ø> (ø)
src/Database/Models/Domain.php	100.00% <ø> (ø)
src/Database/Models/ImpersonationToken.php	100.00% <ø> (ø)
src/Exceptions/TenantCouldNotBeIdentifiedById.php	0.00% <0.00%> (ø)
...ions/TenantCouldNotBeIdentifiedByPathException.php	33.33% <ø> (+4.76%)	:arrow_up:
...nantCouldNotBeIdentifiedByRequestDataException.php	0.00% <ø> (ø)
...ns/TenantCouldNotBeIdentifiedOnDomainException.php	33.33% <ø> (+4.76%)	:arrow_up:
src/Facades/GlobalCache.php	100.00% <ø> (ø)
... and 84 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[stancl]

Are all of these three points solved? https://github.com/archtechx/tenancy/pull/793#issuecomment-1167829755

[stancl]

@abrardev99 could you also fix the missing health check in docker-compose.yml?

[abrardev99]

Are all of these three points solved? https://github.com/archtechx/tenancy/pull/793#issuecomment-1167829755

All of the points are completed, but I'm not sure about 2. I think the cache key is unique based on composer.json. key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }}.

[abrardev99]

could you also fix the missing health check in docker-compose.yml?

I don't think docker-compose.yml has a missing health check in this branch.

[stancl]

The missing healthcheck is in 3.x I think.

[stancl]

But if that part of the code isn't affected by this PR, you can just ignore it and resolve it later. Will probably be easier and result in fewer conflicts.

[abrardev99]

The missing healthcheck is in 3.x I think.

It's not in 3.x either. It's in the master branch where mssql changes got merged, and I think only mssql has missing health checks.

[stancl]

I think the cache key is unique based on composer.json.

This would mean that it's not unique on each PHP/Laravel version.

[stancl]

Actually, if it's based on composer.lock, it might be unique since different PHP & Laravel versions will install different packages. Though I'd still like to be sure about this, or that there is some completely unique random value in each composer.lock. Which I don't think there is.

[abrardev99]

Did you see my reply to the review? I explained there.

https://github.com/archtechx/tenancy/pull/793#discussion_r931897332

[stancl]

I did see it. My comment addresses the potential issues with using composer.lock which is what that reply mentioned.

[abrardev99]

I see. So then, we can use the "matrix" config with different Laravel and PHP versions.

[stancl]

Not 100% sure now, do we want this in 3.x for any specific reason @abrardev99? Or can we change the target to master?

I'd rather make this change as part of v4.

[stancl]

So then, we can use the "matrix" config with different Laravel and PHP versions.

Is this implemented now?

[abrardev99]

Is this implemented now?

Not yet.

can we change the target to master? I'd rather make this change as part of v4.

Yes, we can make it to v4.

[abrardev99]

Is this implemented now?

It's got implemented.

[stancl]

Can you resolve the conflicts? For CONTRIBUTING.md you can just use what's in master, for ci.yml you can use what's in this PR (though do check if there are any things in master that we may want to add back).

[abrardev99]

@stancl The checks are failing. Looks like master got some changes like enum, which works only on PHP 8.1. So we can't really test below 8.1.

So now, checks pass only when the PHP version is 8.1 on L8 and L9. We can't test L6 because of PHP version limitations.

Let me know what you want now to make it compatible with previous PHP versions, Or you might want to keep it as it is now.

[stancl]

Yeah, master is meant to only run on 8.1 :) v4 will use the latest version of Laravel and PHP.

[abrardev99]

I see. Then checks are good now.

[stancl]

Seems like this improves the CI speed by ~30 seconds.

Previous:

Current:

I think if we moved all of the php setup into the container, as I just wrote in the review above, the time should be cut down to around a half.

[abrardev99]

I think if we moved all of the php setup into the container, as I just wrote in the review above, the time should be cut down to around a half.

What! we are down to 7s

[stancl]

Do we need the setup PHP step at all? If we are using the Docker container? What does the step actually do?

[abrardev99]

Do we need the setup PHP step at all?

Yes, otherwise, it does not find a composer. Also, I think we need these for coverage.

[stancl]

Can you expand on that? I'm not sure what you mean.

I think the php version should come from an env var passed to the docker container, I don't see what the setup php step would be needed for.

What does the step actually do?

[abrardev99]

I don't see what the setup php step would be needed for.

I tried removing it, and I got a composer not found error.

Can we install composer in Dockerfile and simply use it in CI?

[stancl]

Very likely we can. That's why I asked you what the step is exactly for. You should find that. If it only sets up composer, then we can simply set up composer in the docker image, properly handle the PHP version variable, and simplify the CI steps here and make the Dockerfile completely usable for local tests as well.

The bold part is also a point of this PR, we don't just want to make CI work, we want the image to work great for local tests.

Right now, we have a Dockerfile that makes the tests always run in the same context. It's used in the CI and it's used locally. In this PR, we are improving the Dockerfile and optimizing the CI for speed, but we still want the Dockerfile to be the authoritative environment in which the tests can be run locally.

[stancl]

@abrardev99 I addressed the open review on Basecamp. I'll merge this now and if any changes are needed you can open a new PR 👍🏻

Thanks a lot for your work on this @abrardev99 @erikgaal!