Skip to content

ci: Fixed an issue where GitHub Actions could not connect to PostgreSQL and MySQL. #12475

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

Closed

Conversation

KentarouTakeda
Copy link
Contributor

@KentarouTakeda KentarouTakeda commented Oct 19, 2023

Since the pdo-pgsql tests are only run on CircleCI when merged into the master branch, some contributors seem to be working on setting up CircleCI themselves. for example: #12448

As with any other test, it may be more convenient to run it in GitHub Actions.


(Added on 2023/10/20 0:00 JST)

Fix details

All connections to PostgreSQL and MySQL from CI running on GitHub Actions were failing. With this fix, the following tests will now work correctly.

  • pdo_pgsql
  • pdo_mysql
  • mysqli
    • Hold. When fixed, tests that are no longer skipped will fail.
  • pgsql

There were no problems with SQL Server, Oracle, etc., but only PostgreSQL and MySQL could not be connected. The difference is that they use GitHub Actions Service Containers.

Previous implementation:

  • The service name of Service Containers was set as the connection destination from the test.

If the test itself is running inside Service Containers, it can be connected without any problems by name resolution between containers. But the tests are running outside of Service Containers.

Implementation after fixing:

  • After exposing the connection port to the database from Service Containers to the host to localhost,
  • Fixed to set localhost also for connection from test.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Ah, good eye. This is an oversight. However, it doesn't seem like this PR works. The number of executed tests doesn't change. Looking at the junit.out.xml, the pdo_pgsql tests are still skipped.

PDO::__construct(): Argument #1 ($dsn) must be a valid data source name

@KentarouTakeda
Copy link
Contributor Author

Sorry. I had overlooked it. I'll try to fix it!

@KentarouTakeda KentarouTakeda force-pushed the ci-test-pdo-pgsql-on-github branch from 9db720e to fe114c6 Compare October 19, 2023 10:51
@KentarouTakeda KentarouTakeda marked this pull request as draft October 19, 2023 13:28
@KentarouTakeda KentarouTakeda changed the title ci: add the environment variable for testing pdo-pgsql on github [WIP]ci: Fixed an issue where GitHub Actions could not connect to PostgreSQL and MySQL. Oct 19, 2023
@KentarouTakeda KentarouTakeda force-pushed the ci-test-pdo-pgsql-on-github branch from fe114c6 to 58c09d0 Compare October 19, 2023 15:27
@KentarouTakeda KentarouTakeda marked this pull request as ready for review October 19, 2023 16:27
@KentarouTakeda
Copy link
Contributor Author

Thank you for the review. I'm sorry for taking up so much of your time.

When I corrected the point raised by @iluuu1994, I realized that there was a completely different cause than I originally thought. As a result of fixing that, many tests other than pdo_pgsql were fixed.

The concept of "fixing a test that wasn't working so that it works" remains the same, but the scope and approach of the fix has changed significantly, so we changed the subject of the pull request and added details to the text.

@KentarouTakeda KentarouTakeda changed the title [WIP]ci: Fixed an issue where GitHub Actions could not connect to PostgreSQL and MySQL. ci: Fixed an issue where GitHub Actions could not connect to PostgreSQL and MySQL. Oct 19, 2023
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

I'm sorry for taking up so much of your time.

Please don't worry about that. Volunteer work is appreciated!

Huh. Apparently ext/mysqli, ext/pdo_mysql and ext/pdo_pgsql are all not running in GitHub actions and nobody noticed. Your PR does not yet fix the ext/mysqli tests. Do you want to have a look or should I?

@KentarouTakeda
Copy link
Contributor Author

Your PR does not yet fix the ext/mysqli tests. Do you want to have a look or should I?

I tried to fix it as much as I could, but I couldn't fix everything.
Could you please take a look?

https://github.com/KentarouTakeda/php-src/actions/runs/6582187009/job/17883329034

========DIFF========
##### COMMENTED This is correct on Linux
001- Warning: mysqli_real_connect(): This stream does not support SSL/crypto in %s
##### COMMENTED On Windows, this is the output
001+ Warning: failed loading cafile stream: `x509.ca' in D:\a\php-src\php-src\ext\mysqli\tests\gh8978.php on line 10
Cannot connect to MySQL using SSL
done!
========DONE========
FAIL Bug GH-8267 (Invalid error message when connection via SSL fails) [D:\a\php-src\php-src\ext\mysqli\tests\gh8978.phpt]

It seems that the error message has changed in some version, but the output seems to vary depending on the OS and execution environment. I don't know how to deal with this...(Since we are not testing the OS, I feel like I should just look at Warning.)

Since it doesn't pass the test, I've marked it as a draft. I'm fine with either reverting the mysqli test and doing it with another pull request, or making some modifications!

@KentarouTakeda KentarouTakeda marked this pull request as draft October 20, 2023 03:58
@KentarouTakeda
Copy link
Contributor Author

KentarouTakeda commented Oct 20, 2023

If this commit is OK, I checked the all pass of the test in my repository.
KentarouTakeda@a108e5b

@iluuu1994 iluuu1994 closed this in f42cef6 Oct 20, 2023
@iluuu1994
Copy link
Member

@KentarouTakeda Seems to have worked with a few tweaks. Thank you!

@KentarouTakeda
Copy link
Contributor Author

Thank you for your very kind review and merge!

I hope everyone's development experience with php-src will be improved. Let me help you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants