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

Fixes #5334 - removes traces of Elastic Search #5578

Merged
merged 1 commit into from
Nov 17, 2015

Conversation

cfouant
Copy link
Member

@cfouant cfouant commented Nov 10, 2015

No description provided.

@beav
Copy link
Contributor

beav commented Nov 12, 2015

There may still be a couple of traces in the backup/restore script and in the ping test.

LGTM aside from that though 🐨

@cfouant cfouant force-pushed the elasticsearch-cleanup branch 2 times, most recently from d82032b to 1667458 Compare November 13, 2015 18:50
@ehelms
Copy link
Member

ehelms commented Nov 14, 2015

If this is designed to remove the last bits of ES, please update this to 'fixes' and also update the Redmine ticket to better describe what this is fixing.

@ehelms
Copy link
Member

ehelms commented Nov 14, 2015

module Provider
class ReindexSubscriptions < ElasticSearch::Abstract
class ReindexSubscriptions < Actions::EntryAction
middleware.use Actions::Middleware::KeepCurrentUser
Copy link
Member

Choose a reason for hiding this comment

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

Blank line after this to separate logical chunks of code for readability.

@ehelms
Copy link
Member

ehelms commented Nov 14, 2015

@johnpmitsch
Copy link
Contributor

Looks like here and here can be removed in host collections controller

@cfouant
Copy link
Member Author

cfouant commented Nov 16, 2015

@johnpmitsch - should all traces of System.index be removed? I note at least one other.

@ehelms
Copy link
Member

ehelms commented Nov 17, 2015

Can you also update the PR message to say fixes?

@johnpmitsch
Copy link
Contributor

@cfouant I think so, I assume you mean https://github.com/Katello/katello/blob/master/lib/katello/tasks/clean_backend_objects.rake#L19 -- maybe @ehelms can confirm that line is ok to remove?

@cfouant cfouant changed the title Refs #5334 - removes traces of Elastic Search Fixes #5334 - removes traces of Elastic Search Nov 17, 2015
@ehelms
Copy link
Member

ehelms commented Nov 17, 2015

@cfouant Yea, instances of System.index can be removed. In general, .index was an elasticsearch concept.

@ehelms
Copy link
Member

ehelms commented Nov 17, 2015

Based on IRC conversation, you will want to remove the webmock removal commit -- that should also get the tests back to green.

@ehelms
Copy link
Member

ehelms commented Nov 17, 2015

ACK for me -- as @jlsherrill did a lot of the original ES work, I'd like him to do final ACK on this

@jlsherrill
Copy link
Member

ACK from me 👍 So long ES, we hardly knew ye 🚢

cfouant added a commit that referenced this pull request Nov 17, 2015
Fixes #5334 - removes traces of Elastic Search
@cfouant cfouant merged commit 72eac6b into Katello:master Nov 17, 2015
@cfouant cfouant deleted the elasticsearch-cleanup branch November 17, 2015 23:45
@mccun934
Copy link
Member

I know this is closed but I wanted to comment with a 👍 and 👏

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.

7 participants