-
Notifications
You must be signed in to change notification settings - Fork 930
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
don't attempt to update a package if no versions could be found #8502
Conversation
As you explained, this is more of a "could not find any versions for this package in the remote source" situation, rather than a "this package is update to date" situation, which is the effect of early returning Like in 38d5044, I'd rather raise an explicit, but unexpected, error that explains the situation and makes it easier to figure out the real culprit. My understanding is that if you had introduced this code from the beginning, it would've been really hard to notice this bug. Maybe in this case it's not a very bad bug (you just have a "phantom dependency" around that dependabot thinks it's valid and up to date), but I'd rather be sure that the update checker is never fed with dependencies that don't really exist, and I believe failing hard is an easy way to achieve that. |
f5b046d
to
72c721b
Compare
defe51a
to
8268aa4
Compare
@deivid-rodriguez I re-worked this change to only report a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. If RepositoryFinder understood nuget.configs <packageSourceMapping>, it could potentially reduce the number of dependeny urls that had to be checked. Future enhancement maybe.
Good call, that'll improve the other lookups, too. I'm going to add one or 2 more test scenarios, just to be safe, then I think this is ready for final approval. |
This will prevent reporting a dependency that doesn't exist on any of the repos and therefore can't be updated.
8268aa4
to
f3548ca
Compare
Updated to stub out more network calls instead of pulling from a file. Reduced changed lines from 36k to 300. |
d29b3aa
to
26f1b0a
Compare
name: name, | ||
version: version, | ||
package_manager: "nuget", | ||
requirements: [requirement] | ||
) | ||
|
||
# only include dependency if one of the sources has it | ||
return unless dependency_has_search_results?(dependency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider logging the dependency and why it was skipped. It might be hard to track down why a certain dependency is missing without a log message here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit different than how most of the other ecosystems do it, but let's give it a shot. I like that the calls are cached so they don't have to be made again during the UpdateChecker step.
I deployed this and a new exception popped up:
Reproducible with: |
Over the last 3 hours the NilClass error has dropped dramatically. Now the KeyError is frequent, but it's an order of magnitude less than the NilClass error was. This is definitely an improvement, I'll go ahead and merge this so we can keep moving forward. |
@jakecoffman PR for |
This partially fixes the issue:
This can happen if querying NuGet for the available package versions returns an empty set and can happen if the package source doesn't contain that package.
The fix is to only report dependencies where at least one of the package repositories reported at least one search result.