Implement #194: remove `latest` composer deps from default generated pipelines #197

Ocramius · 2023-03-03T17:56:25Z

Fixes #194

This change removes latest from default composer dependencies in the generated CI matrix.

The rationale is that latest dependencies tend to break our builds, and we usually run @renovate-bot or @dependabot on our repositories, keeping both composer.json and composer.lock fairly updated.

Because of this kind of disciplined approach, we can assume that latest dependencies are already regularly tested by refreshing composer.lock regularly, and further testing with explicit composer update is considered risky, because it moves into unexplored land, breaking builds that are otherwise quite stable.

We also don't want to break builds by contributors, or builds that upgrade perfectly safe to upgrade dependencies, just because a specific latest upstream dependency broke, and we didn't really touch it.

The final objective is that CI can break, but only in commits that change composer.json and composer.lock, and those are handled every day by automation.

Therefore, the default testing approach will cover lowest and locked dependencies, which both move much less than latest, and don't cause instability.

Q	A
Documentation	no
Bugfix	no
BC Break	yes
New Feature	no
RFC	yes
QA	yes

Ocramius · 2023-03-03T17:57:47Z

tests/configuration-explicit-job-latest-dependency-set/matrix.json

@@ -0,0 +1,10 @@
+{


I'm checking whether latest still works here: will see.

Ocramius · 2023-03-03T18:21:12Z

@boesing do you have any tip about where to document the dependency upgrade endorsement, perhaps? 🤔

Ocramius · 2023-03-03T18:29:45Z

Added some docs meanwhile, unsure if sufficient :)

@renovate-bot

…erated pipelines This change removes `latest` from default composer dependencies in the generated CI matrix. The rationale is that `latest` dependencies tend to break our builds, and we usually run @renovate-bot or @dependabot on our repositories, keeping both `composer.json` and `composer.lock` fairly updated. Because of this kind of disciplined approach, we can assume that `latest` dependencies are already regularly tested by refreshing `composer.lock` regularly, and further testing with explicit `composer update` is considered risky, because it moves into unexplored land, breaking builds that are otherwise quite stable. We also don't want to break builds by contributors, or builds that upgrade perfectly safe to upgrade dependencies, just because a specific `latest` upstream dependency broke, and we didn't really touch it. The final objective is that CI can break, but only in commits that change `composer.json` and `composer.lock`, and those are handled every day by automation. Therefore, the default testing approach will cover `lowest` and `locked` dependencies, which both move much less than `latest`, and don't cause instability.

internalsystemerror · 2023-03-03T19:55:18Z

You make a sound argument and I'm all in favour. However by removing latest entirely, how do we then cover the cases where:

Package A depends on Package B ^1.0
Package A composer.lock is generated on php: 8.1.0
Package B 1.0.0 requires php: 8.1.0
Package B 1.1.0 requires php: 8.2.0 AND more importantly, introduces a bug...

Users on PHP 8.2 will require package A which will indirectly install package B 1.1.0, finding there is a bug in "our package". Without running the latest checks, we would not have any visibility over this, as renovate would be running the lockfile maintenance on PHP 8.1, and (in this example) since 1.1.0 satisfies the ^1.0 constraint, an update wouldn't be needed.

internalsystemerror · 2023-03-03T19:57:37Z

I don't support this idea myself, but just throwing this out there ... Could the above be solved by multliple lock files, one for each PHP minor?

Edit: sry accidentally clicked comment + close on this.

Ocramius · 2023-03-03T20:29:51Z

@internalsystemerror I agree with you that this leaves uncovered corners, but dependencies varying and breaking otherwise perfectly stable builds is, IMO, more of a priority to fix, especially with the vast number of packages that we have.

This is even more problematic as dependencies keep adding bad side effects, like deprecation warnings.

Effectively this attempts to shield our maintenance efforts from random failures happening due to shifting dependencies, whilst still keeping --prefer-lowest tested.

Also, this will encourage us to upgrade our lowest supported dependency ranges a bit more often 😁

Ocramius · 2023-03-03T20:31:33Z

Could the above be solved by multliple lock files, one for each PHP minor?

I don't see this working with contemporary tooling, sadly.

Xerkus · 2023-03-04T00:20:08Z

Considering users get latest dependencies, I can not see how latest could go away here tbh.

lowest is the one most useless, since it brings dependencies to versions that are not normal to install or update operations.
I disagree with removal of latest outside of locked dependencies.

boesing · 2023-03-04T00:21:44Z

I will check this tomorrow. The only reason why I think this is okayish is, that we have still kinda "latest" checks in the composer.lock Updates from renovate. So we remove side-effects caused by latest versions of packages from contributor PRs but they will be covered by renovate and thus we can tackle these.

At least that is how I understand is the plan, correct?

Xerkus · 2023-03-04T00:25:51Z

We have lock file on whatever lowest php we support, 8.0 at this time, mostly. It won't install dependencies that list PHP 8.1 or 8.2 as lowest. Neither will lowest in the matrix with old dependencies having ranges like ~8.0.

Using either lock file or lowest thus excludes whole range of dependencies from ever appearing in CI until someone complains or minimum PHP is bumped.

boesing · 2023-03-04T00:33:19Z

Then we might want to have an additional - maybe daily - executed action which checks latest. having no checks at all would be a nogo from me. At least somehow we should verify if some downstream package starts doing weird things. I'd be fine if that would be a daily executed action which is not executed in PRs. I am +1 of reducing contributor pain.

Xerkus · 2023-03-04T00:39:51Z

Now that I think about it, with the platform version set in composer file, do we even install possible latest versions for newer PHP versions?

Xerkus · 2023-03-04T00:57:17Z

~~I checked random CI run and for 8.1 latest with platform set to 8.0.99 log shows~~

Upgrading phpunit/phpunit (9.5.28 => 9.6.4)

~~Considering phpunit 10 wants 8.1 as minimum I have my answer.~~
derp. I was sure composer.json allowed phpunit 10. Renovate PR bumping it was not merged.

Then we might want to have an additional - maybe daily - executed action which checks latest.

Latest possible dependencies are not installed in pipelines at all currently. Except when platform is ignored. Separate run might need to be added which strips platform set in composer and runs with latest.

boesing · 2023-03-04T09:06:15Z

@Xerkus afaik, the CI Pipeline removes the config.platform.php setting for lowest and latest runs.

https://github.com/laminas/laminas-continuous-integration-action/blob/a39482668bb41479b0c8f8cdd2a3369e5a0c8628/entrypoint.sh#L106

Ocramius · 2023-03-04T09:51:09Z

Then we might want to have an additional - maybe daily - executed action which checks latest. having no checks at all would be a nogo from me. At least somehow we should verify if some downstream package starts doing weird things. I'd be fine if that would be a daily executed action which is not executed in PRs. I am +1 of reducing contributor pain.

Having nightly runs for latest sounds really good: I mostly want to remove latest from any patches by contributors working on unrelated stuff.

Ocramius · 2023-03-04T10:03:46Z

This doesn't seem to have unanimous support at all, so I'd say that we can drop it from 2.0.0 and plan it for a new major afterwards.

Alternatively useful to discuss during Monday's Laminas TSC meeting /cc @laminas/technical-steering-committee

boesing · 2023-03-04T13:21:55Z

Id be on it if we have an alternative way to have latest tested. But without an alternative, that would feel weird.
I wonder if we can detect who is opening the PR and decide whether or not to add latest to the matrix.

Could be opt-in via config and Auto-enabled for laminas/mezzio/api-tools orgs.

weierophinney · 2023-03-04T13:50:31Z

The main rationale I can think of for latest, considering renovate is bumping the composer.lock regularly, is for testing deps that have unions. This allows us to lock against the lower or a middle range, giving us a reasonable test against the full set of supported versions.

But those cases are niche. A config setting that allows opt-in to that approach (lowest/locked/latest) would suffice.

boesing · 2023-03-04T17:38:57Z

src/config/app.ts

@@ -13,7 +13,6 @@ export const ACTION = 'laminas/laminas-continuous-integration-action@v1';
 export enum ComposerDependencySet {
    LOWEST = 'lowest',
    LOCKED = 'locked',
-    LATEST = 'latest',


I would not remove this. It should be possible to generate Jobs with latest, but that wont be the default tho besides opt-in.

I still see this being necessary as something has to check latest, just not every PR.

boesing · 2023-03-04T17:40:15Z

src/config/app.ts

@@ -111,10 +110,10 @@ function discoverPhpVersionsForJob(job: JobDefinitionFromFile, config: Config):

 function discoverComposerDependencySetsForJob(job: JobDefinitionFromFile, config: Config): ComposerDependencySet[] {
    const dependencySetFromConfig = job.dependencies
-        ?? (config.lockedDependenciesExists ? ComposerDependencySet.LOCKED : ComposerDependencySet.LATEST);
+        ?? (config.lockedDependenciesExists ? ComposerDependencySet.LOCKED : ComposerDependencySet.LOWEST);


not sure if this makes sense at all. if there is no lock-file, latest would be installed in projects as well. since we are not affected in the laminas components (as we do have lockfiles), I'd say there is no need to touch this.

boesing · 2023-03-04T17:41:38Z

src/config/app.ts


    if (isAnyComposerDependencySet(dependencySetFromConfig)) {
-        return [ ComposerDependencySet.LOWEST, ComposerDependencySet.LATEST ];
+        return [ ComposerDependencySet.LOWEST ];


I wonder if we should keep this as it is explicitly required from a manually written config entry. If some component does not need/want latest, they can remove that from their config.

boesing · 2023-03-04T17:42:20Z

src/config/app.ts

@@ -295,7 +294,7 @@ function createJobsForTool(
    if (tool.executionType === ToolExecutionType.STATIC) {
        const lockedOrLatestDependencySet: ComposerDependencySet = config.lockedDependenciesExists
            ? ComposerDependencySet.LOCKED
-            : ComposerDependencySet.LATEST;
+            : ComposerDependencySet.LOWEST;


same as above. If there is no lockfile, having lowest would be super weird imho.

boesing · 2023-03-04T17:45:43Z

README.md

+      By operating on locked dependencies, you will be able to pinpoint the exact dependency
+      change that caused a test regression.
+      We endorse regularly updating dependencies with automation like [renovate](https://github.com/renovatebot) or
+      [dependabot](https://github.com/dependabot).


what renovate/dependabot config entry would be required to have these updates?

besides this, it seems that lockfile updates do not properly handle latest. So we might want to have an additional action to be registered - or we go the way that we do only run latest for auto-contributions such as dependabot/renovate.

The lockfile maintenance is as if you run composer update on the PHP version specified in config.platform.php, so we won't get updates that require a higher minimum PHP version.

internalsystemerror · 2023-03-04T19:46:11Z

I like the idea of running latest nightly/weekly to still cover the outliers 👍. We could discuss how to handle that specifically during the TSC meeting on Monday. With that in mind, I'm in favour of proceeding with this PR as is.

See laminas/laminas-ci-matrix-action#197 for reasoning

Ocramius added BC Break Enhancement Feature Removal labels Mar 3, 2023

Ocramius added this to the 2.0.0 milestone Mar 3, 2023

Ocramius self-assigned this Mar 3, 2023

Ocramius added the Awaiting Author Updates label Mar 3, 2023

Ocramius commented Mar 3, 2023

View reviewed changes

Ocramius force-pushed the feature/#194-stop-testing-latest-dependencies-by-default branch from 51415e2 to 80449e9 Compare March 3, 2023 18:29

Ocramius force-pushed the feature/#194-stop-testing-latest-dependencies-by-default branch from 80449e9 to 3c7580a Compare March 3, 2023 18:32

Ocramius added Awaiting Maintainer Response and removed Awaiting Author Updates labels Mar 3, 2023

Ocramius requested review from boesing, internalsystemerror, weierophinney and Xerkus March 3, 2023 18:32

Ocramius force-pushed the feature/#194-stop-testing-latest-dependencies-by-default branch from 3c7580a to 6d71ad6 Compare March 3, 2023 18:33

internalsystemerror closed this Mar 3, 2023

internalsystemerror reopened this Mar 3, 2023

Xerkus approved these changes Mar 4, 2023

View reviewed changes

Ocramius added the RFC Request for Comment; proposal for major new feature or changes. label Mar 4, 2023

boesing requested changes Mar 4, 2023

View reviewed changes

boesing mentioned this pull request Apr 4, 2023

Update dependency psr/http-message to v2 mezzio/mezzio-problem-details#39

Merged

1 task

Lctrs added a commit to Lctrs/php-library-skeleton that referenced this pull request May 25, 2023

fix(ci): remove tests on highest composer deps

9f6b44b

See laminas/laminas-ci-matrix-action#197 for reasoning

Lctrs mentioned this pull request May 25, 2023

fix(ci): remove tests on highest composer deps Lctrs/php-library-skeleton#839

Merged

Lctrs added a commit to Lctrs/psalm-psr-container-plugin that referenced this pull request Jan 10, 2024

chore(ci): remove tests on highest composer deps

a6f2cee

See laminas/laminas-ci-matrix-action#197 for reasoning

Lctrs mentioned this pull request Jan 10, 2024

chore(ci): remove tests on highest composer deps Lctrs/psalm-psr-container-plugin#997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement #194: remove `latest` composer deps from default generated pipelines #197

Implement #194: remove `latest` composer deps from default generated pipelines #197

Ocramius commented Mar 3, 2023

Ocramius Mar 3, 2023

Ocramius commented Mar 3, 2023

Ocramius commented Mar 3, 2023

internalsystemerror commented Mar 3, 2023

internalsystemerror commented Mar 3, 2023 •

edited

Ocramius commented Mar 3, 2023 •

edited

Ocramius commented Mar 3, 2023

Xerkus commented Mar 4, 2023

boesing commented Mar 4, 2023

Xerkus commented Mar 4, 2023

boesing commented Mar 4, 2023

Xerkus commented Mar 4, 2023

Xerkus commented Mar 4, 2023 •

edited

boesing commented Mar 4, 2023

Ocramius commented Mar 4, 2023

Ocramius commented Mar 4, 2023

boesing commented Mar 4, 2023

weierophinney commented Mar 4, 2023

boesing Mar 4, 2023

boesing Mar 4, 2023

boesing Mar 4, 2023

boesing Mar 4, 2023

boesing Mar 4, 2023

internalsystemerror Mar 4, 2023

internalsystemerror commented Mar 4, 2023

Implement #194: remove latest composer deps from default generated pipelines #197

Are you sure you want to change the base?

Implement #194: remove latest composer deps from default generated pipelines #197

Conversation

Ocramius commented Mar 3, 2023

Ocramius Mar 3, 2023

Choose a reason for hiding this comment

Ocramius commented Mar 3, 2023

Ocramius commented Mar 3, 2023

internalsystemerror commented Mar 3, 2023

internalsystemerror commented Mar 3, 2023 • edited

Ocramius commented Mar 3, 2023 • edited

Ocramius commented Mar 3, 2023

Xerkus commented Mar 4, 2023

boesing commented Mar 4, 2023

Xerkus commented Mar 4, 2023

boesing commented Mar 4, 2023

Xerkus commented Mar 4, 2023

Xerkus commented Mar 4, 2023 • edited

boesing commented Mar 4, 2023

Ocramius commented Mar 4, 2023

Ocramius commented Mar 4, 2023

boesing commented Mar 4, 2023

weierophinney commented Mar 4, 2023

boesing Mar 4, 2023

Choose a reason for hiding this comment

boesing Mar 4, 2023

Choose a reason for hiding this comment

boesing Mar 4, 2023

Choose a reason for hiding this comment

boesing Mar 4, 2023

Choose a reason for hiding this comment

boesing Mar 4, 2023

Choose a reason for hiding this comment

internalsystemerror Mar 4, 2023

Choose a reason for hiding this comment

internalsystemerror commented Mar 4, 2023

Implement #194: remove `latest` composer deps from default generated pipelines #197

Implement #194: remove `latest` composer deps from default generated pipelines #197

internalsystemerror commented Mar 3, 2023 •

edited

Ocramius commented Mar 3, 2023 •

edited

Xerkus commented Mar 4, 2023 •

edited