Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
replace direct access of hidden indices with system indices api #12279
replace direct access of hidden indices with system indices api #12279
Changes from all commits
9da547f
19bb1e1
9dd7c73
ddc1b70
ad24c5f
4b0a134
96006cb
329e37a
cba4387
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this might be simpler if we kept the response object in this class, and had methods like 'config_exists?(pipeline_id)',
get_pipeline_config(pipeline_id)
andget_pipeline_settings(pipeline_id)
. This would avoid having to pass around theresponse
andfetcher
objectsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This question to me is if we want to further refactor the existing code or the goal is to apply the new API in a manageable way. This involved thirty lines of code, mainly moving
get_pipeline
and the fetcher in a OO way, which is not a big change. At the same time, the readability of the current version is quite similar to the existing one. I am opened to the suggestion. Do you think we should refactor the code?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure there is a huge amount of refactoring to the existing code either way, beyond what is already present; you already have the method
fetcher.get_single_pipeline_setting(response, pipeline_id)["pipeline"]
which could change to something like
fetched_config.get_pipeline_settings(pipeline_id)
, although I do realize that there is more work to be done in the extra classes that you have added.I'm comfortable either way, what you have appears to be functionally correct after running this code locally against Elasticsearch
7.9
and8.0