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

DX: lock SCA tools for PR builds #6217

Merged
merged 1 commit into from Jan 12, 2022

Conversation

keradus
Copy link
Member

@keradus keradus commented Jan 3, 2022

We recently had an issue that master started to fail because on an update of PHPStan, starting detecting new issues.
To avoid confusion for contributors, we suggested on maintainers meeting to lock the dev-tools and updated them occasionally (eg once per Q). Same time, for master build, that is observed mostly by maintainers, we can still look for bleeding-edge version of SCA tools

@keradus
Copy link
Member Author

keradus commented Jan 3, 2022

@coveralls
Copy link

coveralls commented Jan 3, 2022

Coverage Status

Coverage remained the same at 93.027% when pulling e9a48fa on keradus:dev-tools-lock into efeab76 on FriendsOfPHP:master.

Copy link
Contributor

@kubawerlos kubawerlos left a comment

Choose a reason for hiding this comment

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

I'd vote for removing ^ from dev-tools/composer.json instead of having dev-tools/composer.lock committed, 4k lines of unnecessary noise.

"platform-overrides": {
"php": "7.4"
},
"plugin-api-version": "2.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Update your Composer :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@keradus ping ☝🏼

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 don't see any value in analysing the lock file on this moment and it doesn't bother anyone but you

Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't bother anyone but you

I thought you were better than this... In fact I was hoping you would update your Composer and discover by yourself this new option of Composer 2.2 and update composer.json accordingly.

Copy link
Member Author

@keradus keradus Jan 10, 2022

Choose a reason for hiding this comment

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

you quoted part of my line. let me rephrase (the whole line) - I'm not aware of benefits for doing so, if any - you didn't bother to mention any initially, and everyone else already approved the PR.

You want to be picky? tell ppl what to update.
You want to share some interesting idea? give the reasoning from start - it works way better ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

tell ppl what to update.

I did :trollface:

Update your Composer :)

Well, it will stop working in July, so I guess no need to update it right now :P

@keradus
Copy link
Member Author

keradus commented Jan 4, 2022

I'd vote for removing ^ from dev-tools/composer.json instead of having dev-tools/composer.lock committed

it won't lock the deep-deps (dependencies of dependency)

Copy link
Member

@localheinz localheinz left a comment

Choose a reason for hiding this comment

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

👍

@kubawerlos
Copy link
Contributor

it won't lock the deep-deps (dependencies of dependency)

Obviously, but why do we need to lock them? If some dependency needs to be locked that mean it should be explicitly added to composer.json.

@keradus
Copy link
Member Author

keradus commented Jan 4, 2022

I wouldn't share that claim, myself.
You don't know which dependency needs to be locked until you explicitly experience issue because of it (like first-time contributor creates a PR and it failed on SCA for nothing related to given PR). Locking all SCA deps with lock-file prevents that, locking only 1st level dependencies (not using lock file) will only minimise the risk

@SpacePossum
Copy link
Contributor

Thank you @keradus.

@SpacePossum SpacePossum merged commit 3f7a35d into PHP-CS-Fixer:master Jan 12, 2022
@keradus keradus deleted the dev-tools-lock branch January 12, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants