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

[bug 1211961] Fix opensearch #2669

Merged
merged 1 commit into from
Oct 6, 2015
Merged

[bug 1211961] Fix opensearch #2669

merged 1 commit into from
Oct 6, 2015

Conversation

willkg
Copy link
Member

@willkg willkg commented Oct 6, 2015

opensearch was totally busted. Partially from a lint-fixing round in
2014 and partially from upgrading to a new ElasticUtils that "dealt"
with array values.

I fixed those two issues and also redid the template so that it's not
using the Site framework because that makes things difficult to test.

However, it's still got problems:

  1. it builds a different search than simple search does and since it's
    in a different place in the code, it's been unmaintained and thus
    things like templates pop up in the results; we should merge the
    two
  2. none of the tests test the results of the opensearch search
    suggestions so when it breaks, we have no idea--further, there's
    a bunch of other stuff that should get tested

I didn't want to do either of those now. Instead I tested it manually
with curl. I'll look into doing them later as I work through refactoring
all the Elasticsearch code.

Relatedly, I think we should nix the opensearch altogether since few
people use it (as evidenced by the fact it was busted for sooooo long).

r?

opensearch was totally busted. Partially from a lint-fixing round in
2014 and partially from upgrading to a new ElasticUtils that "dealt"
with array values.

I fixed those two issues and also redid the template so that it's not
using the Site framework because that makes things difficult to test.

However, it's still got problems:

1. it builds a different search than simple search does and since it's
   in a different place in the code, it's been unmaintained and thus
   things like templates pop up in the results; we should merge the
   two

2. none of the tests test the results of the opensearch search
   suggestions so when it breaks, we have no idea--further, there's
   a bunch of other stuff that should get tested

I didn't want to do either of those now. Instead I tested it manually
with curl. I'll look into doing them later as I work through refactoring
all the Elasticsearch code.

Relatedly, I think we should nix the opensearch altogether since few
people use it (as evidenced by the fact it was busted for sooooo long).
@mythmon
Copy link
Contributor

mythmon commented Oct 6, 2015

Looks good to me. Thanks a lot for jumping on this!

mythmon pushed a commit that referenced this pull request Oct 6, 2015
@mythmon mythmon merged commit 51d92ac into mozilla:master Oct 6, 2015
@willkg willkg deleted the 1211961-opensearch branch October 8, 2015 18:41
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.

2 participants