add load_dotenv() to ensure environment variables are loaded

[MustaphaU]

This PR adds load_dotenv() to the instructions for using the Python Elasticsearch client to ensure that environment variables, such as ELASTIC_PASSWORD, are properly loaded.

Without calling load_dotenv(), environment variables defined in a .env file are not loaded into the runtime environment. As a result:

• The call:

os.getenv('ELASTIC_PASSWORD')

returns None

this leads to the following error when initializing the Elasticsearch client:

client = Elasticsearch(
    "http://localhost:9200",
    basic_auth=(username, password)
)

Error:

TypeError: sequence item 1: expected str instance, NoneType found

[cla-checker-service]

💚 CLA has been signed

[elasticsearchmachine]

Pinging @elastic/es-docs (Team:Docs)

[PeteGillinElastic]

Hi, thanks for your Elasticsearch PR.

I'm routing this to the docs team for review.

However, I will make a drive-by comment: I think the docs as they stand work for the basic case. They tell you to set ELASTIC_PASSWORD in the environment, and the sample code then reads it from the environment. As I understand it, the load_dotenv call is only needed if you choose to put the password in a .env file instead.

So my 2¢ is that we shouldn't be issuing a blanket advice to make this call (which brings in a dependency on the dotenv module) when it isn't needed in the case being described. I'm not sure whether it's worth mentioning the alternative of using a .env file and using dotenv to load it, or whether that's just general knowledge that Python devs who've chosen to go the .env route would be expected to know. (I'm not a Python dev.)

Also, does the change suggested not require an import to make it work? (Again, I'm not a Python dev, so I could be wrong there.)

[MustaphaU]

Hi @PeteGillinElastic thank you for the feedback and for routing this to the docs team for review.

You are correct that using load_dotenv() would require an import statement (from dotenv import load_dotenv). I have included that in the PR.

My intention with the load_dotenv() suggestion was to provide an option for users who might choose to use the autogenerated credentials saved in the .env file during the initial setup. The README mentions that a random password is created for the elastic user, which is displayed at the end of the installation and stored in the .env file.

If the Python client section in the README assumes the use of these credentials, it might be helpful to explicitly mention saving the password in an environment variable. For example, after running the setup command:

curl -fsSL https://elastic.co/start-local | s

which outputs:

🎉 Congrats, Elasticsearch and Kibana are installed and running in Docker!

🌐 Open your browser at http://localhost:5601

   Username: elastic
   Password: PASSWORD

🔌 Elasticsearch API endpoint: http://localhost:9200
🔑 API key: SOMEAPIKEY

Learn more at https://github.com/elastic/start-local

We could add a note like:

• Save the PASSWORD in an environment variable:

export ELASTIC_PASSWORD=PASSWORD

This would clarify how to use the autogenerated credentials with the Python client. However, if my interpretation is incorrect, I’m happy to revise the PR or the suggestion should be simply ignored. Thanks again for your review!

[PeteGillinElastic]

Ah, thanks for the additional info, your suggestion makes more sense now. I confess that I have never actually used start-local. As an ES dev, I almost always build from source.

It still seems to me as though using dotenv is not strictly required when using start-local. You could manually set the environment variable to the value the script reports when it runs. You could source the .env script and then export the variable. Or you could use dotenv. (And probably there are several other options.) We could mention one or more option here. I'd still prefer not to imply that users must use dotenv, bringing in the dependency. I'd defer to others on the correct balance of completeness vs simplicity in the docs.

Incidentally, it looks like the .env generated by start-local sets ES_LOCAL_PASSWORD, not ELASTIC_PASSWORD. At least, it did when I ran it just now. So I think the instructions would need a further change to make them work this way.

[PeteGillinElastic]

In case it wasn't clear: I do agree with your basic point that we should make some change here. The section on the python client immediately follows the start-local section, and the user does have to do something in between the start-local and using the python client code given, and it's unhelpful that we currently leave the reader to figure that step out for themselves.

I'm just not certain which option(s) we should be suggesting 🙁.

[MustaphaU]

Ah yes, you're absolutely right -- the autogenerated password is saved to ES_LOCAL_PASSWORD variable in the .env file. I also agree that it is not mandatory to use dotenv to load environment variables. It is simply one of many possible approaches, but I understand the concern about introducing a dependency unnecessarily.

Given that the Python client section immediately follows the start-local setup, perhaps, we can add a note to export ELASTIC_PASSWORD for users who have used start-local, like so:

Source the .env file to load the environment variables and export the elastic user password

source .env
export ELASTIC_PASSWORD=$ES_LOCAL_PASSWORD

Again, there are probably other better options but this approach at least avoids the dotenv dependency.

[PeteGillinElastic]

Ah yes, you're absolutely right -- the autogenerated password is saved to ES_LOCAL_PASSWORD variable in the .env file. I also agree that it is not mandatory to use dotenv to load environment variables. It is simply one of many possible approaches, but I understand the concern about introducing a dependency unnecessarily.

Given that the Python client section immediately follows the start-local setup, perhaps, we can add a note to export ELASTIC_PASSWORD for users who have used start-local, like so:

Source the .env file to load the environment variables and export the elastic user password

source .env
export ELASTIC_PASSWORD=$ES_LOCAL_PASSWORD

Again, there are probably other better options but this approach at least avoids the dotenv dependency.

That general approach sounds reasonable. A few thoughts...

1. It seems less confusing if we change the example code to read the ES_LOCAL_PASSWORD variable instead of having two variables with different names available in different contexts. We'd have to update the example code and the text above it. Then export ELASTIC_PASSWORD=$ES_LOCAL_PASSWORD can be simplified to export ES_LOCAL_PASSWORD (which will just export the value it's already set to).
2. Rather than prescribing a specific method, maybe we should say something like "You should set the ES_LOCAL_PASSWORD environment variable, e.g. by running source .env, and export it, e.g. by running export ES_LOCAL_PASSWORD". I think that gives a step-by-step for users who need it while allowing users to figure out what way of doing something different which works for their use-case.
3. We should probably change the name of the environment variable used in the curl example above to match (and maybe throw in a similar suggestion that they can set it with source .env... there's no need for an export in this case).

[PeteGillinElastic]

@MustaphaU I just saw your latest commit. I like the approach. Please comment when you're happy that it's ready for review.

Unless anyone from the docs team happens to jump in soon, I'm happy to take this on myself, I think I have enough context now.

[MustaphaU]

Thank you @PeteGillinElastic I’m okay with the current state of the changes, and it’s ready for review. Please let me know if there’s anything else I can adjust.

[PeteGillinElastic]

LGTM. Thanks for the contribution, and for engaging in the discussion so helpfully.

I can take care of getting this merged now.

[PeteGillinElastic]

buildkite test this

[PeteGillinElastic]

This is now merged. Congratulations on your first contribution to Elasticsearch, @MustaphaU !

[MustaphaU]

This is now merged. Congratulations on your first contribution to Elasticsearch, @MustaphaU !

Thank you @PeteGillinElastic ! 🎉