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

CI workflow updates #1179

Merged
merged 1 commit into from
Jun 27, 2024
Merged

CI workflow updates #1179

merged 1 commit into from
Jun 27, 2024

Conversation

lukebakken
Copy link
Collaborator

@lukebakken lukebakken commented Jun 3, 2024

  • Start RabbitMQ using docker run so a restart is not necessary.
  • Use latest toxiproxy version

@ramunasd I've copied some scripts from here to set up GitHub actions for tests. In another PR, I'll do the same for Windows tests, and remove the AppVeyor build.

@lukebakken lukebakken self-assigned this Jun 3, 2024
@lukebakken lukebakken added this to the 3.7.0 milestone Jun 3, 2024
@lukebakken lukebakken force-pushed the lukebakken/ci-updates branch 2 times, most recently from 45ed215 to ec333e9 Compare June 3, 2024 13:58
@lukebakken lukebakken force-pushed the lukebakken/ci-updates branch 2 times, most recently from 23c39a6 to 04cf897 Compare June 3, 2024 15:49
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 73.17%. Comparing base (efccda5) to head (3d318f6).

Files Patch % Lines
PhpAmqpLib/Wire/IO/StreamIO.php 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1179      +/-   ##
============================================
- Coverage     73.21%   73.17%   -0.05%     
- Complexity     1047     1048       +1     
============================================
  Files            39       39              
  Lines          3129     3131       +2     
============================================
  Hits           2291     2291              
- Misses          838      840       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lukebakken lukebakken changed the title Small CI workflow updates CI workflow updates Jun 3, 2024
@lukebakken lukebakken force-pushed the lukebakken/ci-updates branch 3 times, most recently from 111b5a1 to f09c02d Compare June 3, 2024 16:38
@lukebakken lukebakken requested a review from ramunasd June 3, 2024 16:43
@lukebakken lukebakken marked this pull request as ready for review June 3, 2024 16:43
@ramunasd
Copy link
Member

ramunasd commented Jun 3, 2024

why?

@lukebakken
Copy link
Collaborator Author

It removes the RabbitMQ restart, also, the script I added can be run locally to get a test environment going. I can add that to the docs.

@ramunasd
Copy link
Member

ramunasd commented Jun 4, 2024

local test environment was working fine 🤷

@lukebakken
Copy link
Collaborator Author

@ramunasd I'm going to add instructions for running a local test environment.

How do you do it? I'm assuming using docker compose up but I'm a bit mystified by what the php service does.

@ramunasd
Copy link
Member

Well yes, docker-compose deos the job. That script You wrote basically is overengineered version of docker-compose :)
php container has php itself and can run tests.

@lukebakken
Copy link
Collaborator Author

You wrote basically is overengineered version of docker-compose :)

Ha, yes, but it does work well for GitHub actions.

php container has php itself and can run tests

Yep, but ... how? I just ran docker compose up and then docker compose exec php /bin/bash. When I run ./vendor/bin/phpunit I received an error that PhP 7.1 was unsupported, and that 7.3 was required.

So, I updated docker/php/Dockerfile to use 7-php. Working on this further, and I will document it as well.

@lukebakken
Copy link
Collaborator Author

Using 7-php in docker/php/Dockerfile does not work for running tests, as there are missing functions. Giving 8-php a try now.

@lukebakken
Copy link
Collaborator Author

OK great, down to two errors:

There were 2 errors:

1) PhpAmqpLib\Tests\Unit\Wire\IO\SocketIOTest::connect_ipv6
PhpAmqpLib\Exception\AMQPIOException: Error Connecting to server (111): Connection refused

/src/PhpAmqpLib/Wire/IO/SocketIO.php:82
/src/tests/Unit/Wire/IO/SocketIOTest.php:33

2) PhpAmqpLib\Tests\Unit\Wire\IO\StreamIOTest::connect_ipv6
PhpAmqpLib\Exception\AMQPIOException: stream_socket_client(): Unable to connect to tcp://[::1]:5672 (Connection refused)

/src/PhpAmqpLib/Wire/IO/StreamIO.php:95
/src/tests/Unit/Wire/IO/StreamIOTest.php:59

I'll commit my latest changes.

@ramunasd
Copy link
Member

docker dev image is based on oldest supported php version, which is 7.2
Very likely your local setup already had compose.lock file built with newer php version, that's why You get an error.
so docker compose down and rm composer.lock does the trick.
In CI we don't have such issues as workspace is always clean.

@lukebakken
Copy link
Collaborator Author

Thank you for the explanation, I'm sure that was the issue. I will document it.

* Start RabbitMQ using `docker run` so a restart is not necessary.
* Use latest `toxiproxy` version
* Save RabbitMQ logs
* Ensure RabbitMQ logs to file
* Set up RabbitMQ and Toxiproxy using the same scripts as the .NET client
* Update CONTRIBUTING.md to include instructions for starting RabbitMQ
* Add make targets to start up docker test env, and a target to run tests via docker
@lukebakken
Copy link
Collaborator Author

@ramunasd no hurry, take a look when you have time. Thanks!

@ramunasd
Copy link
Member

I see Your efforts to make thing better, but this looks overcomplicated. I don't understand why You want to remove php container from docker-compose? It was really fine before, kind of community standard to use docker-compose directly.

If You want have ability to rebuild images, then it can solved with simple docker-compose build --no-cache as separate make target 🤷

If You really want, go ahead, but I wont use it :)

@lukebakken lukebakken merged commit d1af284 into master Jun 27, 2024
11 of 12 checks passed
@lukebakken lukebakken deleted the lukebakken/ci-updates branch June 27, 2024 19:41
@lukebakken
Copy link
Collaborator Author

lukebakken commented Jun 27, 2024

If You really want, go ahead, but I wont use it :)

Noted! The changes in this PR are similar to those I have done in the .NET client and other client libraries that Team RabbitMQ maintains. It'll make it easier for me to remember how to do things like run tests when I switch between client libraries.

And of course if there are issues with my changes going forward let me know and I will fix them ASAP!

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