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

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

Open
rosiel opened this issue Oct 5, 2021 · 5 comments
Open

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

rosiel opened this issue Oct 5, 2021 · 5 comments
Labels
Repository: Crayfish Commons Issues pertaining to the repository:https://github.com/Islandora/Crayfish-Commons

Comments

@rosiel
Copy link
Member

rosiel commented Oct 5, 2021

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

adam-vessey commented Oct 6, 2021

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

@rosiel
Copy link
Member Author

rosiel commented Oct 6, 2021

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:

  • including lock files in microservices is fine, recommended even, so that we are all on the same versions.
  • we do not have a process for updating these lock files, and if we continue on this path, we should.

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.

@kstapelfeldt kstapelfeldt added the Repository: Crayfish Commons Issues pertaining to the repository:https://github.com/Islandora/Crayfish-Commons label Oct 7, 2021
@nigelgbanks
Copy link
Member

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

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

nigelgbanks added a commit to nigelgbanks/CrayFits that referenced this issue Nov 6, 2022
- Removed lock file
- Updated test matrix

For reasons behind removing the lock file see:
https://islandora.slack.com/archives/CM5PPAV28/p1659631615201049
Islandora/documentation#1908 (Removal approved)

Instead we'll provide lock files in isle and ansible deployments.
nigelgbanks added a commit to nigelgbanks/CrayFits that referenced this issue Nov 6, 2022
- Removed lock file
- Updated test matrix

For reasons behind removing the lock file see:
https://islandora.slack.com/archives/CM5PPAV28/p1659631615201049
Islandora/documentation#1908 (Removal approved)

Instead we'll provide lock files in isle and ansible deployments.
nigelgbanks added a commit to nigelgbanks/Crayfish that referenced this issue Nov 6, 2022
- Removed lock file
- Updated test matrix

For reasons behind removing the lock file see:
https://islandora.slack.com/archives/CM5PPAV28/p1659631615201049
Islandora/documentation#1908 (Removal approved)

Instead we'll provide lock files in isle and ansible deployments.

Additionally moving to drop tests for 7.3 as it is no longer supported
by Drupal 9.4 and up.
nigelgbanks added a commit to nigelgbanks/Crayfish that referenced this issue Nov 6, 2022
- Removed lock file
- Updated test matrix
- Fixed tests to work on php 7.4, 8.0, 8.1

For reasons behind removing the lock file see:
https://islandora.slack.com/archives/CM5PPAV28/p1659631615201049
Islandora/documentation#1908 (Removal approved)

Instead we'll provide lock files in isle and ansible deployments.

Additionally moving to drop tests for 7.3 as it is no longer supported
by Drupal 9.4 and up.
nigelgbanks added a commit to nigelgbanks/Crayfish that referenced this issue Dec 13, 2022
- Removed lock file
- Updated test matrix
- Fixed tests to work on php 7.4, 8.0, 8.1

For reasons behind removing the lock file see:
https://islandora.slack.com/archives/CM5PPAV28/p1659631615201049
Islandora/documentation#1908 (Removal approved)

Instead we'll provide lock files in isle and ansible deployments.

Additionally moving to drop tests for 7.3 as it is no longer supported
by Drupal 9.4 and up.
whikloj pushed a commit to Islandora/Crayfish that referenced this issue Dec 15, 2022
* Attempt to get working under PHP 8.1
- Removed lock file
- Updated test matrix
- Fixed tests to work on php 7.4, 8.0, 8.1

For reasons behind removing the lock file see:
https://islandora.slack.com/archives/CM5PPAV28/p1659631615201049
Islandora/documentation#1908 (Removal approved)

Instead we'll provide lock files in isle and ansible deployments.

Additionally moving to drop tests for 7.3 as it is no longer supported
by Drupal 9.4 and up.

* Removed debug code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Repository: Crayfish Commons Issues pertaining to the repository:https://github.com/Islandora/Crayfish-Commons
Projects
Development

No branches or pull requests

5 participants