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

Don't recommend --full-index on errors #6493

Merged
merged 3 commits into from
Mar 17, 2023

Conversation

deivid-rodriguez
Copy link
Member

What was the end-user or developer problem that led to this PR?

The --full-index flag is slow since the resolver needs to fetch the remote gemspec every time it needs to figure out dependencies of a gem, which happens a lot during resolution.

Plus it's still potentially vulnerable to dependency confusion issues.

Also, some of this recommendations did not even make sense as far as I understand.

Finally these days the dependency APIs are reliable enough, and there are implicit fallbacks in place anyways.

What is your fix for the problem, implemented in this PR?

Stop recommending --full-index.

Make sure the following tasks are checked

@deivid-rodriguez deivid-rodriguez added this pull request to the merge queue Mar 16, 2023
@deivid-rodriguez deivid-rodriguez removed this pull request from the merge queue due to a manual request Mar 16, 2023
@deivid-rodriguez
Copy link
Member Author

I think we need https://github.com/ruby/setup-ruby/releases/tag/v1.144.1 to fix the mswin build. Will add it to this PR.

@deivid-rodriguez
Copy link
Member Author

I created a separate PR to bump the ruby/setup-ruby action with Dependabot, but I now realized I think this is actually fixed already because in the case of mswin we use the ruby/setup-ruby-pkgs action which depends on a "rolling v1 version" of ruby/setup-ruby, so the fix has been picked up automatically I think. Let me try it though.

@deivid-rodriguez deivid-rodriguez added this pull request to the merge queue Mar 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 17, 2023
@hsbt hsbt added this pull request to the merge queue Mar 17, 2023
@hsbt hsbt removed this pull request from the merge queue due to the queue being cleared Mar 17, 2023
The debug message suggests retrying using `--full-index`, but the retry
is happening automatically. Just log that we are falling back to the
full index, like we do with other errors.
We're actually already using the full index here, so it makes no sense
to suggest retrying the same thing.
I've never seen this error in real life, and if it was happening, I
think it's either some server side issue that would need to be fixed or
some transient issue. We should move away from the full index, since
it's slow, so let's stop recommending it.
@deivid-rodriguez deivid-rodriguez added this pull request to the merge queue Mar 17, 2023
@deivid-rodriguez deivid-rodriguez merged commit d5b20aa into master Mar 17, 2023
@deivid-rodriguez deivid-rodriguez deleted the dont-recommend-full-index branch March 17, 2023 20:16
deivid-rodriguez added a commit that referenced this pull request Mar 20, 2023
Don't recommend `--full-index` on errors

(cherry picked from commit d5b20aa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants