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

Meta: Don't update Cargo.toml when updating semver-compatible dependencies #3541

Closed
rami3l opened this issue Nov 23, 2023 · 10 comments · Fixed by #3564
Closed

Meta: Don't update Cargo.toml when updating semver-compatible dependencies #3541

rami3l opened this issue Nov 23, 2023 · 10 comments · Fixed by #3564
Assignees
Labels
meta This issue is related to project management.

Comments

@rami3l
Copy link
Member

rami3l commented Nov 23, 2023

IMO we should not merge Cargo.toml changes that bump the semver requirement.

Originally posted by @djc in #3540 (comment)

cc killercup/cargo-edit#552

@rami3l
Copy link
Member Author

rami3l commented Nov 23, 2023

I did the research and found out that we can actually set rangeStrategy to update-lockfile in .github/renovate.json, like in this example.

However, I'm not sure if this supports cargo or not. The docs page only says bundler, composer, npm, yarn, terraform and poetry are supported.

What I suggest if we want to get this fixed is:

@djc Does that sound okay to you?

@djc
Copy link
Contributor

djc commented Nov 23, 2023

Sounds good to me!

@rami3l rami3l self-assigned this Nov 26, 2023
@rami3l
Copy link
Member Author

rami3l commented Nov 26, 2023

Looks like the WIP that this issue will depend on (at renovatebot/renovate#23968) has been closed.

I'm not that familiar with TypeScript but I guess I could possibly give it a try in December or so.
Or, if anyone else wants to work on it, please go ahead!

@rbtcollins
Copy link
Contributor

Yah, its a missing feature in renovate :/.

What I do here is just close pr's that are within the existing semver range except if they are security patches where we want to force the newer dep always.

@rami3l
Copy link
Member Author

rami3l commented Dec 6, 2023

The patch I picked up (renovatebot/renovate#25983) has been successfully merged!

@djc
Copy link
Contributor

djc commented Dec 6, 2023

Great! Do we need to modify your configuration or can this issue be closed?

@rami3l
Copy link
Member Author

rami3l commented Dec 7, 2023

@djc We need to modify the config (I've changed #3541 (comment) to a to-do list). However I'll be very busy today, maybe later?

@rami3l
Copy link
Member Author

rami3l commented Dec 8, 2023

Reopening due to the concern raised in #3569 (comment).

@rami3l
Copy link
Member Author

rami3l commented Dec 13, 2023

    // NOTE: Partial versions like '1.2' don't get converted to '^1.2'
    // because isVersion('1.2') === false
    // In cargo and in npm 1.2 is equivalent to 1.2.* so it is correct behavior.

It seems suspect? I don't know how isVersion() is used exactly. 1.2 in Cargo should be treated as ^1.2.0.

Originally posted by @djc in #3569 (comment)

@djc I think isVersion means "is a valid semver string", for more examples you can refer to https://www.npmjs.com/package/semver.

For our specific case, on https://npm.runkit.com/semver you can see:

> var semver = require("semver")
> !!semver.valid('1.2')
false

@rami3l
Copy link
Member Author

rami3l commented Dec 13, 2023

renovatebot/renovate#26263 has been created.

Update: The PR has been merged. Closing this issue hopefully for the one last time.

@rami3l rami3l closed this as completed Dec 13, 2023
@rami3l rami3l added the meta This issue is related to project management. label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta This issue is related to project management.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants