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 `outdated` listing regression from cc355865 #5105

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@lucasmazza
Contributor

lucasmazza commented Oct 20, 2016

Fixes #4979, by using match_platform over comparing platform values directly, as suggested by @segiddins on the original thread.

I couldn't figure out how to test this, as the outdated tests are end to end tests that don't replicate this issue, as it seems the the EndpointSpecification objects are only present when reaching rubygems.org API.

/cc @chrismo

@segiddins

This comment has been minimized.

Show comment
Hide comment
@segiddins

segiddins Oct 20, 2016

Member

You'll have to use a spec where you install using the compact index or the dependency API via the :artifice option to bundle or install_gemfile

Member

segiddins commented Oct 20, 2016

You'll have to use a spec where you install using the compact index or the dependency API via the :artifice option to bundle or install_gemfile

@lucasmazza

This comment has been minimized.

Show comment
Hide comment
@lucasmazza

lucasmazza Oct 21, 2016

Contributor

@segiddins I experimente with :artifice => "compact_index" on some of the outdated specs without any luck - maybe I'm doing it wrong and this bug might require a more specific test case or the API using during the tests don't have the same bug as the live one where we don't have the platform information available.

Contributor

lucasmazza commented Oct 21, 2016

@segiddins I experimente with :artifice => "compact_index" on some of the outdated specs without any luck - maybe I'm doing it wrong and this bug might require a more specific test case or the API using during the tests don't have the same bug as the live one where we don't have the platform information available.

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Oct 22, 2016

Contributor

☔️ The latest upstream changes (presumably #5061) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

homu commented Oct 22, 2016

☔️ The latest upstream changes (presumably #5061) made this pull request unmergeable. Please resolve the merge conflicts.

@swrobel

This comment has been minimized.

Show comment
Hide comment
@swrobel

swrobel Nov 3, 2016

@homu the conflicts seem to be resolved. Can we get a merge here? This bug is pretty confounding!

swrobel commented Nov 3, 2016

@homu the conflicts seem to be resolved. Can we get a merge here? This bug is pretty confounding!

@segiddins

This comment has been minimized.

Show comment
Hide comment
@segiddins

segiddins Nov 3, 2016

Member

@swrobel this needs test coverage before it can be merged

Member

segiddins commented Nov 3, 2016

@swrobel this needs test coverage before it can be merged

@chrismo

This comment has been minimized.

Show comment
Hide comment
@chrismo

chrismo Nov 14, 2016

Member

I wrote up some tests for this - actually did some refactoring first to make some small specs possible. I'll push up later.

Member

chrismo commented Nov 14, 2016

I wrote up some tests for this - actually did some refactoring first to make some small specs possible. I'll push up later.

@chrismo

This comment has been minimized.

Show comment
Hide comment
@chrismo

chrismo Nov 14, 2016

Member

Closing in favor of PR #5171.

Member

chrismo commented Nov 14, 2016

Closing in favor of PR #5171.

@chrismo chrismo closed this Nov 14, 2016

@lucasmazza lucasmazza deleted the lucasmazza:lm-match-platform-outdated-command branch Nov 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment