Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 9 commits into from
Oct 26, 2021

Conversation

Yozhef
Copy link
Contributor

@Yozhef Yozhef commented Jan 13, 2021

No description provided.

Copy link
Contributor

@Jean85 Jean85 left a 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.

@Yozhef Yozhef force-pushed the allowCISymfony5.0 branch from a21451e to abdcd85 Compare January 13, 2021 20:06
@ste93cry
Copy link
Contributor

If we speak about theory, the variables that we must consider for the matrix are:

  • version of Symfony
  • version of the dependencies (lowest and highest)
  • version of PHP
  • OS

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

@Jean85
Copy link
Contributor

Jean85 commented Jan 14, 2021

Due to the Symfony BC promise, it should be superfluous to test all those versions though.
Also, I'm not sure about OS, since we shouldn't have any OS-dependent handling in this bundle, whereas in the base SDK we have something more low-level that could, and in fact we test it.

@Yozhef
Copy link
Contributor Author

Yozhef commented Jan 14, 2021

@Jean85
I think it's important to check now (Symfony 4.4.* / Symfony 5.0. *) together with PHP versions 7.4 and 8.0. in the future the most popular number of projects.

@Jean85
Copy link
Contributor

Jean85 commented Jan 14, 2021

@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:

  • 3.4
  • 4.4
  • 5.x
  • prefer-lowest (which is forcibly 3.4)

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!

@Yozhef
Copy link
Contributor Author

Yozhef commented Jan 14, 2021

@Jean85 I think that's enough, but ready to listen to what you suggest to change? Now we have:
Symfony 5.0.* PHP 8.0
Symfony 5.0.* PHP 7.4
Symfony 4.4.* PHP 8.0
Symfony 3.4 PHP 7.2
Symfony 4.4.* PHP 7.4
PHP 8.0
PHP 7.4
PHP 7.3
PHP 7.2
PHP 7.2 - lower

@Jean85
Copy link
Contributor

Jean85 commented Jan 14, 2021

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:

  • PHP 8.0 (implicitly selects latest SF 5.x)
  • PHP 7.4 (implicitly selects latest SF 5.x)
  • PHP 7.3 (implicitly selects latest SF 5.x)
  • PHP 7.2 (implicitly selects latest SF 5.x)
  • PHP 7.4 SF 4.4
  • PHP 7.4 SF 3.4
  • PHP 7.2 prefer lowest

For a grand total of 7, to double it with OS variation if we want. 14 jobs seems enough to me.

@ste93cry ste93cry force-pushed the allowCISymfony5.0 branch 3 times, most recently from 5fae275 to b08a852 Compare April 12, 2021 20:57
@Jean85 Jean85 changed the base branch from master to develop April 13, 2021 07:30
@Jean85
Copy link
Contributor

Jean85 commented Apr 13, 2021

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.

Copy link
Contributor

@Jean85 Jean85 left a 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.

.github/workflows/tests.yaml Show resolved Hide resolved
.github/workflows/tests.yaml Outdated Show resolved Hide resolved
.github/workflows/tests.yaml Outdated Show resolved Hide resolved
.github/workflows/tests.yaml Outdated Show resolved Hide resolved
@ste93cry ste93cry force-pushed the allowCISymfony5.0 branch 5 times, most recently from b23ae69 to d597479 Compare April 13, 2021 19:44
@Jean85
Copy link
Contributor

Jean85 commented Apr 14, 2021

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 pcov here too.

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

@ste93cry ste93cry changed the base branch from develop to master August 29, 2021 15:20
@ste93cry ste93cry force-pushed the allowCISymfony5.0 branch 3 times, most recently from 1e70bc5 to 195af6d Compare August 29, 2021 15:40
@ste93cry ste93cry force-pushed the allowCISymfony5.0 branch 2 times, most recently from 2aeadaa to 8942234 Compare October 18, 2021 21:40
@ste93cry ste93cry requested a review from Jean85 October 25, 2021 19:21
@ste93cry ste93cry changed the title feat(Symfony Version) add Symfony Version. Update the CI to test against Symfony 5.x and PHP 8.1 Oct 26, 2021
Copy link
Contributor

@ste93cry ste93cry left a 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

@ste93cry ste93cry merged commit 4f79cd2 into getsentry:master Oct 26, 2021
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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.