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(pin): avoid pinning deprecated version #4609

Merged
merged 1 commit into from Oct 14, 2019

Conversation

djcsdy
Copy link
Contributor

@djcsdy djcsdy commented Oct 8, 2019

Closes #4608 (I think, not tested yet).

Looks like the problem was that renovate didn't take deprecation into account when determining the current version (fromVersion) of a dependency.

@djcsdy
Copy link
Contributor Author

djcsdy commented Oct 8, 2019

I ran renovate with this change against djcsdy/wildflower-codec and it did the right thing, so I think this is ready for merge.

 INFO: PR title changed (repository=djcsdy/wildflower-codec, dependencies=bitflags, branch=renovate/rust-pin-dependencies)
       "branchName": "renovate/rust-pin-dependencies",
       "oldPrTitle": "chore(deps): pin rust crate bitflags to =1.0.5",
       "newPrTitle": "chore(deps): pin rust crate bitflags to =1.0.4"

@rarkins
Copy link
Collaborator

rarkins commented Oct 13, 2019

I thought about this more and for npm, we should pin it to the same one as before, even if that one is deprecated. At the same time, I want to avoid any manager-specific code in this worker file because it should ideally be captured within lib/manager/*. Somehow we need a way to behave different for Cargo and npm here.

@@ -379,7 +384,7 @@ function getFromVersion(
config: LookupUpdateConfig,
rangeStrategy: string,
allVersions: string[]
): string {
): string | null {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but it’s an unambiguous improvement since the function was already capable of returning null and it’s helpful for the type to document that fact.

At the moment strictNullChecks is turned off in tsConfig.json for compatibility with the parts of renovate that are still written in JavaScript, but as you convert more of the code to TypeScript you probably want to work towards turning this option on to get better static type checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I wanted to make it clear from the type of the function that

getFromVersion(config, rangeStrategy, nonDeprecatedVersions) ||
      getFromVersion(config, rangeStrategy, allVersions)

works because the first function call returns null if there are no matching non-deprecated versions.

@djcsdy
Copy link
Contributor Author

djcsdy commented Oct 13, 2019

I thought about this more and for npm, we should pin it to the same one as before, even if that one is deprecated. At the same time, I want to avoid any manager-specific code in this worker file because it should ideally be captured within lib/manager/*. Somehow we need a way to behave different for Cargo and npm here.

The behaviour with this change for Cargo and npm is:

  1. If a lock file is present, pin to the version specified in the lockfile, don’t care if it is deprecated.
  2. If there is no lock file, pin to the latest non-deprecated version that satisfies the version range, if possible. This is the same version that npm or Cargo would resolve in this case.
  3. If there is no matching non-deprecated version, pin to the latest deprecated version that satisfies the version range.

Which part of this behaviour is not what you want for npm?

@rarkins
Copy link
Collaborator

rarkins commented Oct 14, 2019

@djcsdy thanks for the explanation. I hadn't realised about point (1) above.

@rarkins rarkins merged commit 35c3474 into renovatebot:master Oct 14, 2019
@renovate-bot
Copy link
Collaborator

🎉 This PR is included in version 19.60.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renovate attempts to pin deprecated version of dependency
3 participants