Islandora / documentation

Contains islandora's documentation and main issue queue.
MIT License
103 stars 71 forks source link

Can we delete composer.lock files in Crayfish, Crayfish-Commons? #1908

Open rosiel opened 2 years ago

rosiel commented 2 years ago

In Crayfish and Crayfish Commons, we have composer.json and composer.lock files.

If we want to use these with Composer as per how Semantic Versioning should work, shouldn't we delete the .lock files?

adam-vessey commented 2 years ago

https://getcomposer.org/doc/01-basic-usage.md#installing-from-composer-lock

Either way, running install when a composer.lock file is present resolves and installs all dependencies that you listed in composer.json, but Composer uses the exact versions listed in composer.lock to ensure that the package versions are consistent for everyone working on your project. As a result you will have all dependencies requested by your composer.json file, but they may not all be at the very latest available versions (some of the dependencies listed in the composer.lock file may have released newer versions since the file was created). This is by design, it ensures that your project does not break because of unexpected changes in dependencies.

... so it depends on how much we trust those repos at which we point to correctly follow semantic versioning?

... that said, including the composer.lock, we could probably make use of something like Dependabot to automatically create PRs when it detects that there are newer versions: https://docs.github.com/en/code-security/supply-chain-security/keeping-your-dependencies-updated-automatically/enabling-and-disabling-version-updates

kstapelfeldt commented 2 years ago

See discussion in notes: https://github.com/Islandora/documentation/wiki/October-6,-2021

rosiel commented 2 years ago

There are some dependabot PRs on Crayfish that do what @adam-vessey says: https://github.com/Islandora/Crayfish/pulls

The discussion @kstapelfeldt pointed to left me thinking:

In the interests of time, we focused on getting that semantic versioning PR in with minimal damage to the existing lock files, but the larger issue still stands - the lock files are getting stale and in some cases could be hazardous. That repo in general needs some love - moving to Symfony, and looking at Dependabot's PRs. Those should probably be addressed, then we can return to this discussion.

nigelgbanks commented 2 years ago

Just to puts some notes here from mucking around.

Depending on the system environment that is used when either creating the first lock file or updating it can change the generated lock files contents.

So for example if the system has PHP 7.4 installed composer will choose the package that supports PHP 7.4 when generating the lock file in the cases where a dependencies supports multiple versions like 1.0 || 2.0. If a user then tries to install on a system that has PHP 8.0 it can fail to install since the lock file requires the version of the dependency that supported only PHP 7.4. Even though a dependency does exists to support PHP 8.0. If the lock file were not present it would install successfully.

That being said lock files are great, I just think they should be used in the tools that have control over the system, so in our case Isle and the Ansible Playbook. That way those builds are reproducible, even if they both use a different version of PHP, etc.

seth-shaw-asu commented 2 years ago

General consensus from the Tech Call today is that we can remove composer.lock.