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

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

Open
wants to merge 1 commit into
base: 2.0.x
Choose a base branch
from

Conversation

Ocramius
Copy link
Member

@Ocramius Ocramius commented Mar 3, 2023

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

@@ -0,0 +1,10 @@
{
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'm checking whether latest still works here: will see.

@Ocramius
Copy link
Member Author

Ocramius commented Mar 3, 2023

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

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

Ocramius commented Mar 3, 2023

Added some docs meanwhile, unsure if sufficient :)

…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.
@Ocramius Ocramius force-pushed the feature/#194-stop-testing-latest-dependencies-by-default branch from 3c7580a to 6d71ad6 Compare March 3, 2023 18:33
@internalsystemerror
Copy link
Member

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
Copy link
Member

internalsystemerror commented Mar 3, 2023

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
Copy link
Member Author

Ocramius commented Mar 3, 2023

@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
Copy link
Member Author

Ocramius commented Mar 3, 2023

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
Copy link
Member

Xerkus commented Mar 4, 2023

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
Copy link
Member

boesing commented Mar 4, 2023

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
Copy link
Member

Xerkus commented Mar 4, 2023

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
Copy link
Member

boesing commented Mar 4, 2023

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
Copy link
Member

Xerkus commented Mar 4, 2023

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
Copy link
Member

Xerkus commented Mar 4, 2023

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
Copy link
Member

boesing commented Mar 4, 2023

@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
Copy link
Member Author

Ocramius commented Mar 4, 2023

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
Copy link
Member Author

Ocramius commented Mar 4, 2023

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

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

boesing commented Mar 4, 2023

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
Copy link
Member

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.

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

Choose a reason for hiding this comment

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

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.

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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.


if (isAnyComposerDependencySet(dependencySetFromConfig)) {
return [ ComposerDependencySet.LOWEST, ComposerDependencySet.LATEST ];
return [ ComposerDependencySet.LOWEST ];
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

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).
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Maintainer Response BC Break Enhancement Feature Removal RFC Request for Comment; proposal for major new feature or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants