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

Migrate jobs away from Travis to Github Actions #4310

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Sep 28, 2020

Todo:

  • mssql
  • sqlite
  • ibm_db2 (this one looks really nasty)
  • setup dependency on sqlite job
  • decide what to do about PostgreSQL 9.2 (I can't get it to work)
  • add some 7.3 jobs for MariaDB / PG ?
  • remove scripts from tests/travis?
  • find replacement for cron that runs with dev dependencies
  • make relevant jobs required
  • make sure code coverage hasn't decreased
  • test only the latest version of a platform when not using the latest version of PHP

@greg0ire greg0ire force-pushed the travis-to-gh branch 14 times, most recently from dbc940c to fe06461 Compare September 29, 2020 12:29
@greg0ire greg0ire changed the title Migrate some jobs from Travis to Github Actions Migrate jobs away from Travis to Github Actions Sep 29, 2020
@greg0ire greg0ire requested a review from morozov September 29, 2020 12:46
@greg0ire greg0ire marked this pull request as ready for review September 29, 2020 12:46
@greg0ire
Copy link
Member Author

@localheinz @bendavies please also review 🙏

@bendavies
Copy link
Contributor

very nice

curl https://packages.microsoft.com/keys/microsoft.asc | sudo apt-key add -
curl https://packages.microsoft.com/config/ubuntu/16.04/prod.list | sudo tee /etc/apt/sources.list.d/mssql.list
sudo apt-get update
ACCEPT_EULA=Y sudo apt-get install -qy msodbcsql17 unixodbc unixodbc-dev libssl1.0.0
Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote the equivalent of this at first, only to see that all the packages above were already present somehow.

@morozov
Copy link
Member

morozov commented Sep 29, 2020

This is impressive work, @greg0ire!

@greg0ire
Copy link
Member Author

The issue with the PG 9.2 is that we don't have the logs of the service… Let me try to remove the healtcheck, that way we can proceed with any next step we want. I will add a step that waits 1 mn, and then another that shows the logs.

@greg0ire
Copy link
Member Author

greg0ire commented Sep 29, 2020

Well… I didn't expect this! Looks like the issue is with the healthcheck itself!
UPD: That would be because pg_isready was introduced in 9.3. Before this PR, it was only used for PostgreSQL 11. There was just no healthcheck for lower versions. Not sure what we should do, but let's try to see if it works when I remove the step that sleeps for 1 mn.

@greg0ire
Copy link
Member Author

Works like a charm w/o the delay too… maybe we should merge as is (after a squash and some cleanup of the commented out code of course). If we get issues down the line, we can still add the healtcheck back, and mark the 9.2 build as not required until we figure it out.

@bendavies
Copy link
Contributor

bendavies commented Sep 29, 2020

@greg0ire pg_isready was added in postgresql 9.3

@greg0ire
Copy link
Member Author

Yup that's what I figured in #4310 (comment) 😄

@bendavies
Copy link
Contributor

Ah sorry!
This should work:

--health-cmd "pg_isready || psql -U postgres -c 'SELECT 1'"

@greg0ire
Copy link
Member Author

Ah I think I'm onto something!

@greg0ire
Copy link
Member Author

greg0ire commented Oct 15, 2020

Yaaaaay! +2% coverage! I turned on assertions, it's still code that is better off being run than not run: what if an assertion is wrong?

@greg0ire
Copy link
Member Author

greg0ire commented Oct 15, 2020

I added a todo item to reduce the number of builds according to your recommendation "All other supported PHP versions (7.1, 7.3, nightly), test each driver with the latest version of the platform." @morozov , but please note that MariaDB is not tested with 7.3, so maybe there are more todo items to add

@greg0ire
Copy link
Member Author

I think that will not change things much. Please tell me precisely what rule I should apply to decrease the number of builds. Should we maybe have some rule like "test the lowest supported platform version, and the 3 latest minor versions, but not all versions in between"?

It should be faster, and will result in one fewer CI platform to
maintain.
@morozov
Copy link
Member

morozov commented Oct 15, 2020

I turned on assertions, it's still code that is better off being run than not run: what if an assertion is wrong?

Yeah. CI is the right place to have them enabled.

Please tell me precisely what rule I should apply to decrease the number of builds. Should we maybe have some rule like "test the lowest supported platform version, and the 3 latest minor versions, but not all versions in between"?

I didn't realize we have that many different MariaDB versions tested, so the existing 12 jobs do not technically violate the approach defined in #3347 (comment). I believe we can leave them as is for now and rely on support for some older versions being dropped in 3.0.x.

[…] please note that MariaDB is not tested with 7.3, so maybe there are more todo items to add

I wouldn't bother about adding new jobs right now unless they provide additional code coverage or reveal new issues.

@greg0ire
Copy link
Member Author

Ok, I will now fiddle with the branch settings and finally merge this! 🎉

@morozov
Copy link
Member

morozov commented Oct 15, 2020

Should we maybe have some rule like "test the lowest supported platform version, and the 3 latest minor versions, but not all versions in between"?

Actually what we could do later as optimization is to reduce the set of specific platform versions being tested to the necessary minimum that our code is aware of. I.e. test the oldest version, the newest version, and all versions for which we have version-specific code. For MariaDB, it would be 10.0 (oldest), 10.2 (MariaDb1027Platform), and 10.5 (newest).

If this works, we may want to combine this and the previous rule in a documentation article.

@greg0ire greg0ire merged commit 908cdc6 into doctrine:2.11.x Oct 15, 2020
@greg0ire greg0ire deleted the travis-to-gh branch October 15, 2020 16:20
@greg0ire greg0ire added this to the 2.11.2 milestone Oct 15, 2020
@morozov
Copy link
Member

morozov commented Oct 15, 2020

Awesome work, @greg0ire. You rock!

@thomasrockhu
Copy link

Seriously awesome work @greg0ire for the upload-artifact stuff!

with:
name: "${{ github.job }}-${{ matrix.mariadb-version }}-${{ matrix.extension }}-${{ matrix.php-version }}.coverage"
path: "coverage.xml"

Copy link
Contributor

Choose a reason for hiding this comment

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

extra new line here ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

@bendavies please send a PR :P

Comment on lines +521 to +522


Copy link
Contributor

Choose a reason for hiding this comment

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

and here

@bendavies
Copy link
Contributor

seriously impressive work! 💪

rgrellmann added a commit to Rossmann-IT/dbal that referenced this pull request Mar 7, 2021
Release [2.11.2](https://github.com/doctrine/dbal/milestone/81)

2.11.2
======

- Total issues resolved: **5**
- Total pull requests resolved: **16**
- Total contributors: **10**

Static Analysis
---------------

 - [4353: Update Psalm to 3.17.2 and lock the version used with GitHub Actions](doctrine#4353) thanks to @morozov
 - [4348: Bump Psalm level to 3](doctrine#4348) thanks to @morozov
 - [4332: Static analysis improvements](doctrine#4332) thanks to @morozov
 - [4319: Bump Psalm level to 4](doctrine#4319) thanks to @morozov

Code Style
----------

 - [4346: Minor CS improvement - use ::class for TestCase::expectException input](doctrine#4346) thanks to @mvorisek

 - [4344: Static analysis workflow](doctrine#4344) thanks to @greg0ire
 - [4340: Modernize existing ga](doctrine#4340) thanks to @greg0ire
 - [4309: Use cache action v2](doctrine#4309) thanks to @greg0ire
 - [4305: Move website config to default branch](doctrine#4305) thanks to @SenseException

Improvement,Prepared Statements
-------------------------------

 - [4341: Add Statement::fetchAllIndexedAssociative() and ::iterateIndexedAssociative()](doctrine#4341) thanks to @morozov and @ZaneCEO
 - [4338: Add Statement::fetchAllKeyValue() and ::iterateKeyValue()](doctrine#4338) thanks to @morozov

BC Fix,Query
------------

 - [4330: Fix regression in QueryBuilder::and|orWhere()](doctrine#4330) thanks to @BenMorel

Test Suite
----------

 - [4321: Update PHPUnit to 9.4](doctrine#4321) thanks to @morozov

Columns,SQL Server,Schema Managers
----------------------------------

 - [4315: Fix handling existing SQL Server column comment when other properties change](doctrine#4315) thanks to @trusek

CI
--

 - [4310: Migrate jobs away from Travis to Github Actions ](doctrine#4310) thanks to @greg0ire

BC Fix,Connections
------------------

 - [4308: doctrine#4295 Keep master, slaves, keepReplica params in MasterSlaveConnection](doctrine#4308) thanks to @kralos

New Feature,Prepared Statements
-------------------------------

 - [4289: Add a fetch mode methods for "PDO::FETCH&doctrine#95;KEY&doctrine#95;PAIR"](doctrine#4289) thanks to @tswcode

Bug,SQL Server,Schema
---------------------

 - [3400: Wrong column comment setting command in migrations of SQL Server](doctrine#3400) thanks to @msyfurukawa

# gpg: Signature made Mon Oct 19 04:18:17 2020
# gpg:                using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132
# gpg: Can't check signature: No public key
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants