-
Notifications
You must be signed in to change notification settings - Fork 178
Update the CI to test against Symfony 5.x
and PHP 8.1
#410
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the need for expliciting all those versions. Testing once on every PHP version and at least once for every LTS + latest version seems enough to me.
a21451e
to
abdcd85
Compare
If we speak about theory, the variables that we must consider for the matrix are:
Exploding such matrix into builds will generate a lot of them, that's a fact, but it's also the correct way to ensure that everything works well in all scenarios. Any other solution involving less builds is likely better for resources and time, but it will be also more error-prone. I've seen other repos where they have a lot of checks, and I don't think it would be a problem for us to do the same in terms of numbers, and our CI is anyway fast enough to not take ages to complete |
Due to the Symfony BC promise, it should be superfluous to test all those versions though. |
@Jean85 |
@Yozhef still, the BC promise protects us in that regard: 5.0 is the same as 4.4 without deprecation, and any 5.x will still be compatible with 5.0. So I would test against:
This would mean 4 deps versions + 4 PHP versions already (7.2 -> 8.0) or 8 if you count OS variation, for a total of 12 + static analysis... If you add any version in between we're going over 20-30! |
@Jean85 I think that's enough, but ready to listen to what you suggest to change? Now we have: |
I repeat, testing 5.0 on its own in addition to the latest 5.x doesn't make sense, for the same reason for which we don't test for 4.0. I would test like this:
For a grand total of 7, to double it with OS variation if we want. 14 jobs seems enough to me. |
5fae275
to
b08a852
Compare
I've re-targeted this to develop to reduce the noise in the diff. It could change again if we merge develop into master due to the preparation of the 4.1 release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the requested changes, this produces a big matrix. We should evaluate if all of those are needed, because I don't see enough value in checking all the Symfony versions in each PHP version.
b23ae69
to
d597479
Compare
3.4 is giving us some issues: symfony/symfony#40679 has not been backported there since it's not a security fix, the only way I see to fix this is we should switch to Also, we need to exclude 8.0 + 3.4 because there's no compatible set of components: https://github.com/getsentry/sentry-symfony/pull/410/checks?check_run_id=2336880519 |
d597479
to
8117012
Compare
1e70bc5
to
195af6d
Compare
2aeadaa
to
8942234
Compare
…ity issues with PHP `8.1`
6303c74
to
192463d
Compare
f5e78ce
to
7463e64
Compare
5.x
and PHP 8.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took almost a year to get an approval, but we finally did it 🎉 thanks @Yozhef for working on it initially and sorry if you had to wait so much for the merge
No description provided.