docker icon indicating copy to clipboard operation
docker copied to clipboard

Add optional port parameter to Elasticsearch management functions

Open Be-Mann opened this issue 1 year ago • 4 comments

  • 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.

:wave: 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 :rocket:

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 :clap:

  • [x] Added an optional port parameter to Elasticsearch management functions (status, info, stats, wait).
  • [x] Set up a fallback to the default port 9200 if no port is specified.
  • [x] Updated existing functions to handle the port argument dynamically.
  • [x] Added comments in English to improve code clarity.

Here's how others can test the changes :eyes:

  • Test by running the modified script with and without specifying a port for the Elasticsearch service.
  • Example: pelias elastic status 9201 to check the status of Elasticsearch on port 9201.
  • Ensure that the script works as expected for both default and custom ports.

Be-Mann avatar Oct 23 '24 16:10 Be-Mann

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. ${ELASTIC_HOST:-localhost}

It would also make the code a lot less complex because there are no function calls and passing arguments between scopes.

missinglink avatar Oct 23 '24 16:10 missinglink

Ah yeah good point, there's a pretty strong convention of environment variable configuration in these scripts, which we should keep.

orangejulius avatar Oct 23 '24 16:10 orangejulius

Yeah, putting the discussion of env vs. args aside I feel like doing both will make scripting against the Pelias command inconsistent.

missinglink avatar Oct 23 '24 16:10 missinglink

Looks better, looking at this again it seems the variable $ELASTIC_HOST includes both the host and the port 🤔

${ELASTIC_HOST:-localhost:9200}

So.. I guess this means it should already 'just work' without any new code 🤔 ie. if you export ELASTIC_HOST='localhost:5555' it should already work?

Changing the behaviour of $ELASTIC_HOST to only mean the host and not the port would be a breaking change which we need to be careful of.

missinglink avatar Oct 25 '24 13:10 missinglink

The source branch now contains a bunch of unrelated commits, I'm assuming the original issue was resolved and the port is already modifiable without code changes.

missinglink avatar Dec 09 '24 12:12 missinglink