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

Use new /release/versions/DIST API endpoint #1906

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

mickeyn
Copy link
Contributor

@mickeyn mickeyn commented Jun 4, 2017

Replace the Elasticsearch query sending with a call to the new
API endpoit.

This change also fixes the related tests and simplifies the
way 'verisons' query was called.

Replace the Elasticsearch query sending with a call to the new
API endpoit.

This change also fixes the related tests and simplifies the
way 'verisons' query was called.
@mickeyn mickeyn requested a review from oalders June 4, 2017 23:16
@coveralls
Copy link

Coverage Status

Coverage remained the same at 43.665% when pulling e6151d0 on mickey/release_versions_endpoint into 93aecb9 on master.

Copy link
Member

@oalders oalders left a comment

Choose a reason for hiding this comment

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

👍

@oalders oalders merged commit aefd883 into master Jun 5, 2017
@oalders oalders deleted the mickey/release_versions_endpoint branch June 5, 2017 13:53
@haarg
Copy link
Member

haarg commented Jun 6, 2017

This seems to take one of the asynchronous calls and makes it blocking instead. That doesn't seem like a good change.

The API being used for the async calls is not the best. Maybe we need to change that to make it easier to manage?

@mickeyn
Copy link
Contributor Author

mickeyn commented Jun 6, 2017

@haarg I noticed that, but in this case it didn't seem to matter.

I agree the way we currently do the async calls is way too complicated and hard to maintain.

In this specific case i took a fast, limited query (size=>250) which was also used in other places where async wasn't needed - so i don't believe the performance is affected in a user-noticeable way.

We can restore the versions call into the async calls, but i do want to clean that code first as you suggested.

@mickeyn
Copy link
Contributor Author

mickeyn commented Jun 8, 2017

I'm going to revert this commit as I see it's responsible for tests currently breaking in master (odd...)

@mickeyn
Copy link
Contributor Author

mickeyn commented Jun 8, 2017

@haarg @oalders I reverted this commit and will work on a new one soon
#1911

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants