-
-
Notifications
You must be signed in to change notification settings - Fork 226
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
Add optional port parameter to Elasticsearch management functions #360
base: master
Are you sure you want to change the base?
Conversation
- Introduced an optional port parameter to various Elasticsearch management functions (status, info, stats, wait) to allow flexibility in handling multiple instances running on different ports. - Implemented a fallback mechanism that defaults to port 9200 when no port is specified. - Updated all relevant functions to accept and use the provided port or fall back to the default. - Added comments in English to enhance clarity and maintainability of the script.
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.
Seems good to me. @missinglink?
Thanks for sending us this
I feel like the UX is a bit difficult to understand when the host is set by env var and the port is set by arg. Is there a reason not to use env vars the same way we do with host? ie. It would also make the code a lot less complex because there are no function calls and passing arguments between scopes. |
Ah yeah good point, there's a pretty strong convention of environment variable configuration in these scripts, which we should keep. |
Yeah, putting the discussion of env vs. args aside I feel like doing both will make scripting against the Pelias command inconsistent. |
…st and Port Configuration ### What was specifically improved? - **Consistency in configuration**: Instead of passing the port as an argument or via a function, you now use an environment variable (`ELASTIC_PORT`), just like for the host. This brings consistency, allowing users to set both the host and port using environment variables, which fits better into automated environments like Docker. - **Simplification of code**: Since the `get_elastic_port` function was removed, the code becomes less complex, and reusing environment variables reduces unnecessary argument passing and functionality.
Looks better, looking at this again it seems the variable
So.. I guess this means it should already 'just work' without any new code 🤔 Changing the behaviour of |
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.
Can we please clarify the following before merging this:
- If port can be modified already without any code changes.
- If changing the meaning of
ELASTIC_HOST
would be a breaking change.
👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.
Here's the reason for this change 🚀
This change allows for better flexibility when managing multiple Elasticsearch instances that run on different ports. By adding an optional port parameter to the relevant functions, users can now specify the desired port when interacting with Elasticsearch, improving the usability of the script.
Here's what actually got changed 👏
status
,info
,stats
,wait
).9200
if no port is specified.Here's how others can test the changes 👀
pelias elastic status 9201
to check the status of Elasticsearch on port9201
.