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

rangeStrategy refactor #19802

Open
rarkins opened this issue Jan 12, 2023 · 8 comments
Open

rangeStrategy refactor #19802

rarkins opened this issue Jan 12, 2023 · 8 comments
Assignees
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code

Comments

@rarkins
Copy link
Collaborator

rarkins commented Jan 12, 2023

Describe the proposed change(s).

One thing I'd like to do is remove rangeStrategy=pin and instead revert back to having a boolean pinVersions setting.

This would then leave values replace, update-lockfile, bump and widen.

replace and widen affect how the package file is updated whenever a new version doesn't match the current constraint.

  • For replace, it means to retain the same precision but "shift" the range, e.g. from ^1.0.0 to ^2.0.0.
  • For widen, it means to widen the constraint to satisfy current and new, e.g. from ^1.0.0 to ^1.0.0 || ^2.0.0

bump and update-lockfile affect what to do when the locked version is outdated:

  • For bump, the low range of the package file constraint is increased, e.g. from ^1.0.0 to ^1.0.1. It essentially behaves the same regardless of whether the update is in-range, out of range, major/minor/patch, etc.
  • For update-lockfile, the package file constraint is retained, and the locked version is increased, e.g. from 1.0.0 to 1.0.1. It only really does this for in-range updates, and falls back to replace for out of range

After we extract, we have:

  • currentValue (constraint), which can be a version or a range
  • lockedVersion (optional)

Summary of today:

replace bump update-lockfile widen
No locked - in range Nothing Bump range Nothing Nothing
No locked - out of range Replace Bump range Replace Widen
Locked - in range Nothing Bump range Update lockfile Nothing
Locked - out of range Replace Bump range Replace Widen

bump will essentially always update the package file, which in turn causes an update to the lock file if present.

Whether to update the lockfile when in-range could be valid for both replace and widen, so maybe it should be a boolean option for those. Alternatively we could have it update by default, with updateType=lockfileUpdate and then allow people to enable/disabled based on that.

@rarkins rarkins added type:refactor Refactoring or improving of existing code status:requirements Full requirements are not yet known, so implementation should not be started priority-5-triage labels Jan 12, 2023
@rarkins
Copy link
Collaborator Author

rarkins commented Jan 16, 2023

Copying from WyriHaximus in @19866:

What would you like Renovate to be able to do?

Coming from Dependabot Renovate gives an impressive amount of additional features. My initial experience with Renovate was a bit spotty but that is being resolved through #18715. The thing I would love Renovate to be able to do is allow to pass in an array of range strategies into rangeStrategy rather than having to pick one.

To paint a situation and how Renovate would deal with that:
rangeStrategy is set to the default replace
Language: PHP
PHP version targeting: ^8 || ^7.4
Package vendor/a is currently installed at version v1 targetted with ^1 with has the following PHP versioning constraints: ^8 || ^7.4

Then a vendor/a v2 is released with the following PHP versioning constraints: ^8.1.
Currently Renovate will replace the targeting from ^1 to ^2.

However, what I really want Renovate to do is to update the lockfile and widen if a new major version comes out that supports my targeting ranges. So I can currently either change it to either widen or update-lockfile. However update-lockfile fallback to replace, which is the one I'm avoiding.

If you have any ideas on how this should be implemented, please tell us here.

While reading through the code I realized that having an array of possible strategies to try is a massively complex thing to implement in one manager, not even to mention all of them. So I came to the conclusion that the simplest way to get some of it is to provide a rangeStrategyFallback option or to turn rangeStrategy into an update with two arguments or whatever the maintainers deem best. Where you can specify the fallback which is then injected in calls like this:

rangeStrategy: 'replace',

Is this a feature you are interested in implementing yourself?

Yes

@rarkins
Copy link
Collaborator Author

rarkins commented Jan 16, 2023

Referencing the earlier table, what he's after is this:

image

Which is aligned with my earlier thoughts that:

Whether to update the lockfile when in-range could be valid for both replace and widen,

@internalsystemerror
Copy link
Contributor

I too would like to utilise the above image. You could make rangeStrategy take an object aswell as a string. The object being a more granular way to set the above:

rangeStrategy: {
  unlockedInRange: 'bump',
  unlockedOutOfRange: 'widen',
  lockedInRange: 'update-lockfile',
  lockedOutOfRange: 'widen',
}

@rarkins
Copy link
Collaborator Author

rarkins commented Mar 10, 2023

My gut instinct says to stay away from a nested structure like this. All it really does is shorten the variable names, while adding new ways for people to configure it wrong

@rarkins
Copy link
Collaborator Author

rarkins commented May 1, 2023

Created #21902 (Use pinVersions instead of rangeStrategy=pin)

@internalsystemerror
Copy link
Contributor

internalsystemerror commented May 18, 2023

Should the default Locked - in range strategy for replace and widen both become update-lockfile instead of none? Or is this something you would want as a config variable on its own e.g. updateInRangeLockfiles: bool? I'd be interested in helping with this if I can.

@rarkins
Copy link
Collaborator Author

rarkins commented May 19, 2023

I think it would probably be best to generate such in-range locked updates by default, and then have "is this in-range or not?" metadata which users can use package rules on, e.g. to filter our in-range OR out-of-range updates

@internalsystemerror
Copy link
Contributor

This one might be too much for me. I can't even figure out where this logic is (I've spent a few hours reading the code, reviewing old PRs, trying to figure it out). Best I can gather is that update-lockfile gets changed to replace on specific managers, and not the other way around.

I suspect I'd need much more in depth knowledge to implement this. Sorry.

@rarkins rarkins self-assigned this Oct 1, 2023
@rarkins rarkins added priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:ready and removed priority-5-triage status:requirements Full requirements are not yet known, so implementation should not be started labels Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code
Projects
None yet
Development

No branches or pull requests

2 participants