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

fix(composer): resolve the lockFileMaintenance not working #32384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kayw-geek
Copy link

Changes

Fixed an issue where the lockFileMaintenance configuration was not working for the Composer manager.
Currently, triggering LockFileMaintenance via webhook does not perform any actions.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@kayw-geek kayw-geek changed the title fix: resolve the lockFileMaintenance not working in Composer manager fix(composer): resolve the lockFileMaintenance not working in Composer manager Nov 7, 2024
@rarkins
Copy link
Collaborator

rarkins commented Nov 7, 2024

Do you have a reproduction repo to demonstrate this before/after?

@kayw-geek
Copy link
Author

kayw-geek commented Nov 7, 2024

Hi @rarkins
https://github.com/kayw-geek/24958
This repository can reproduce the current behavior of the Composer manager.
The expected behavior is that composer’s lockFileMaintenance should execute composer update, after which the composer.lock file should change, and the version of guzzlehttp/guzzle should update from 7.9.0 to the latest 7.9.2.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

usally both ways should work. most managers are using the updateType

lockFileConfig.updateType = 'lockFileMaintenance';
lockFileConfig.isLockFileMaintenance = true;

@rarkins should we check all managers to be consistent?

@viceice
Copy link
Member

viceice commented Nov 7, 2024

@kayw-geek
Copy link
Author

maybe related to the grouping? https://github.com/kayw-geek/24958/blob/6a7804b2030da6dcdc53bb71f39aa31378bfa2a5/renovate.json#L17

Should I delete this groupName?

@rarkins
Copy link
Collaborator

rarkins commented Nov 7, 2024

@rarkins should we check all managers to be consistent?

Yes, I suspect there may be some inconsistency here. And we should pay attention to the case where people group lock file maintenance with regular updates. It's not something which we recommend, and it will have different behavior per manager, but many people do it

@viceice
Copy link
Member

viceice commented Nov 7, 2024

@rarkins should we check all managers to be consistent?

Yes, I suspect there may be some inconsistency here. And we should pay attention to the case where people group lock file maintenance with regular updates. It's not something which we recommend, and it will have different behavior per manager, but many people do it

We should issue a warning if lockfile maintenance is grouped with other updates, so users are aware that it's not always working as intended.

I think i know why it's not working here:
First update is a regular update, so isLockFileMaintenance is not copied to branch config, but somehow the updateType is copied. 🤔

@kayw-geek
Copy link
Author

Hi @rarkins @viceice ,

Just wanted to check if there’s any update on the PR, Please let me know if you need anything from my side to move it forward.

@kayw-geek
Copy link
Author

kayw-geek commented Nov 19, 2024

hi @rarkins @viceice,
I would like to confirm if this is a more appropriate approach for Composer update command.

Currently, we use the composer update packageName:packageVersion format to update dependencies. However, if a package version is locked in composer.json (e.g., foo:1.0.0), when Renovate detects version 1.1.0, it attempts to execute the following command to upgrade: composer update foo:1.0.0 --with-dependencies. In reality, this command will fail and produce the following error:

The temporary constraint "1.1.0" for "foo" must be a subset of the constraint in your composer.json (1.0.0)
Run `composer require foo` or `composer require foo:1.1.0` instead to replace the constraint

This situation may also happen when updating the major version.

If my conclusion is correct, should we update the logic to use composer require instead, and depending on the depType, decide whether to add the --dev flag?

Alternatively, should we consider modifying the version constraint in composer.json before running the update command to avoid this issue altogether?

@viceice
Copy link
Member

viceice commented Nov 19, 2024

hi @rarkins @viceice,
I would like to confirm if this is a more appropriate approach for Composer update command.

Currently, we use the composer update packageName:packageVersion format to update dependencies. However, if a package version is locked in composer.json (e.g., foo:1.0.0), when Renovate detects version 1.1.0, it attempts to execute the following command to upgrade: composer update foo:1.0.0 --with-dependencies. In reality, this command will fail and produce the following error:

The temporary constraint "1.1.0" for "foo" must be a subset of the constraint in your composer.json (1.0.0)
Run `composer require foo` or `composer require foo:1.1.0` instead to replace the constraint

This situation may also happen when updating the major version.

If my conclusion is correct, should we update the logic to use composer require instead, and depending on the depType, decide whether to add the --dev flag?

Alternatively, should we consider modifying the version constraint in composer.json before running the update command to avoid this issue altogether?

this seems to be a change in recent composer versions, can you pinpoint the version which breaks renovate?

I think we can change the composer.json before running composer, like we do for other managers.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

see other comment.

@kayw-geek
Copy link
Author

see other comment.

The feature introduces seems like on this commit

@kayw-geek kayw-geek force-pushed the bugfix/fix-lockfile-maintenance-for-composer-manager branch from 43e319c to 0877eb2 Compare November 21, 2024 01:48
@kayw-geek kayw-geek changed the title fix(composer): resolve the lockFileMaintenance not working in Composer manager fix(composer): resolve the lockFileMaintenance not working Nov 21, 2024
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.

3 participants