Add paramters for api_key and basic_auth, resolves #327#869
Add paramters for api_key and basic_auth, resolves #327#869hmaerki wants to merge 3 commits intodbfixtures:mainfrom
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request adds authentication support to pytest-elasticsearch by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_elasticsearch/factories/client.py`:
- Around line 33-36: The fixture currently assumes process has api_key,
basic_auth, request_timeout, and verify_certs which can raise AttributeError for
ElasticSearchExecutor; update the client construction to guard these accesses by
using getattr(process, 'api_key', None), getattr(process, 'basic_auth', None),
getattr(process, 'request_timeout', DEFAULT_TIMEOUT_OR_NONE) and
getattr(process, 'verify_certs', True_or_None) (choose sensible defaults) when
passing values into the Elasticsearch client call so missing attributes won’t
crash before client creation; locate the client creation site where process is
used and replace direct attribute access with guarded getattr calls for those
four symbols.
In `@pytest_elasticsearch/factories/noprocess.py`:
- Around line 42-44: The code currently converts a string es_basic_auth by
partitioning on ":" which turns malformed strings without ":" into (user, ""),
so add explicit validation before conversion: if es_basic_auth is a str ensure
it contains ":" (and not empty parts) and raise a ValueError with a clear
message if it doesn't; if es_basic_auth is already a tuple/list validate it has
exactly two non-empty elements; then only after validation perform the
conversion into (user, password). Refer to the es_basic_auth variable and the
conversion block that uses partition to locate where to add these checks.
In `@README.rst`:
- Around line 174-185: Update the two auth rows in the README.rst table so the
"pytest.ini option" column uses the actual registered option names: replace the
existing api_key entry with elasticsearch_api_key for the API key row and
replace basic_auth with elasticsearch_basic_auth for the basic auth row (look
for the table rows containing "api_key" and "basic_auth" to locate the correct
entries).
- Around line 127-134: Replace the TOML section header in the README snippet to
use pytest's ini-style mapping: change the header from [tool.pytest] to
[tool.pytest.ini_options] so the example mirrors pytest.ini compatibility; keep
the same keys (elasticsearch_host, elasticsearch_port, elasticsearch_basic_auth)
and values but update the header to [tool.pytest.ini_options] in the README.rst
snippet that shows pyproject.toml configuration.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.rstpytest_elasticsearch/config.pypytest_elasticsearch/executor.pypytest_elasticsearch/factories/client.pypytest_elasticsearch/factories/noprocess.pypytest_elasticsearch/plugin.py
README.rst
Outdated
| The same may be achived by adding options to `pyproject.toml` / `pytest.ini`: | ||
|
|
||
| .. code-block:: toml | ||
|
|
||
| [tool.pytest] | ||
| elasticsearch_host="localhost" | ||
| elasticsearch_port="9200" | ||
| elasticsearch_basic_auth="elastic:<your_password>" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pytest pyproject.toml tool.pytest.ini_options configuration
💡 Result:
To configure pytest in pyproject.toml, put options under one of these tables:
1) INI-style (works since pytest 6.0)
# pyproject.toml
[tool.pytest.ini_options]
minversion = "6.0"
addopts = "-ra -q"
testpaths = ["tests", "integration"]
markers = ["slow: long-running tests"]
filterwarnings = ["error", "ignore::DeprecationWarning"]This mirrors what you’d normally write in pytest.ini, but expressed in TOML. (docs.pytest.org)
2) Native TOML types (recommended since pytest 9.0)
[tool.pytest]
minversion = "9.0"
addopts = ["-ra", "-q"]
testpaths = ["tests", "integration"]This avoids INI parsing quirks and lets you use “real” TOML lists for options like addopts. (docs.pytest.org)
Notes that commonly trip people up
pyproject.tomlis only treated as pytest config if it has[tool.pytest]or[tool.pytest.ini_options](with some precedence/behavior changes in newer pytest versions). (docs.pytest.org)- Some plugins also expect you to place their settings into
addopts(example: Allure). (allurereport.org)
Citations:
- 1: https://docs.pytest.org/en/latest/customize.html?utm_source=openai
- 2: https://docs.pytest.org/en/latest/customize.html?utm_source=openai
- 3: https://docs.pytest.org/en/latest/customize.html?utm_source=openai
- 4: https://allurereport.org/docs/pytest-configuration/?utm_source=openai
Clarify the pytest configuration section choice.
Both [tool.pytest.ini_options] and [tool.pytest] are valid in pyproject.toml. Use [tool.pytest.ini_options] if mirroring pytest.ini style (supported since pytest 6.0), or [tool.pytest] for native TOML format (recommended since pytest 9.0). Since the snippet references pytest.ini compatibility, [tool.pytest.ini_options] is the more appropriate choice here.
Proposed fix
- [tool.pytest]
+ [tool.pytest.ini_options]
elasticsearch_host="localhost"
elasticsearch_port="9200"
elasticsearch_basic_auth="elastic:<your_password>"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The same may be achived by adding options to `pyproject.toml` / `pytest.ini`: | |
| .. code-block:: toml | |
| [tool.pytest] | |
| elasticsearch_host="localhost" | |
| elasticsearch_port="9200" | |
| elasticsearch_basic_auth="elastic:<your_password>" | |
| The same may be achived by adding options to `pyproject.toml` / `pytest.ini`: | |
| .. code-block:: toml | |
| [tool.pytest.ini_options] | |
| elasticsearch_host="localhost" | |
| elasticsearch_port="9200" | |
| elasticsearch_basic_auth="elastic:<your_password>" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.rst` around lines 127 - 134, Replace the TOML section header in the
README snippet to use pytest's ini-style mapping: change the header from
[tool.pytest] to [tool.pytest.ini_options] so the example mirrors pytest.ini
compatibility; keep the same keys (elasticsearch_host, elasticsearch_port,
elasticsearch_basic_auth) and values but update the header to
[tool.pytest.ini_options] in the README.rst snippet that shows pyproject.toml
configuration.
b82ff6c to
b171792
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
README.rst (2)
174-185:⚠️ Potential issue | 🟠 MajorAuth rows list incorrect
pytest.inioption names.The "pytest.ini option" column should use the full option names
elasticsearch_api_keyandelasticsearch_basic_authto match the actual registered options inplugin.py.📝 Proposed fix
* - api_key - api_key - --elasticsearch-api-key - - api_key + - elasticsearch_api_key - - - - * - user and password separated by ':' - basic_auth - --elasticsearch-basic-auth - - basic_auth + - elasticsearch_basic_auth - - - -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.rst` around lines 174 - 185, Update the README.rst auth rows so the "pytest.ini option" column uses the exact option names registered in plugin.py: replace occurrences like "api_key" and "basic_auth" with "elasticsearch_api_key" and "elasticsearch_basic_auth" respectively (and ensure any CLI flag examples like "--elasticsearch-api-key" map to the full underscore names shown in pytest.ini); verify the table rows describing the API key and basic auth entries match the option names defined in plugin.py to avoid mismatch.
131-134:⚠️ Potential issue | 🟡 MinorClarify the pytest configuration section choice.
The TOML section header should use
[tool.pytest.ini_options]for pytest.ini-style configuration compatibility (supported since pytest 6.0), not[tool.pytest].📝 Proposed fix
- [tool.pytest] + [tool.pytest.ini_options] elasticsearch_host="localhost" elasticsearch_port="9200" elasticsearch_basic_auth="elastic:<your_password>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.rst` around lines 131 - 134, Update the TOML header from "[tool.pytest]" to "[tool.pytest.ini_options]" so pytest recognizes these ini-style settings (supported since pytest 6.0); leave the keys elasticsearch_host, elasticsearch_port, and elasticsearch_basic_auth unchanged and ensure the values remain quoted as shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_elasticsearch/factories/noprocess.py`:
- Around line 39-58: The factory currently assigns the function arg basic_auth
directly to es_basic_auth, skipping the config-path validation—ensure that if
basic_auth is provided as a string (e.g., "user:password") you parse and convert
it into a (username, password) tuple the same way as the config branch does (use
partition(":") and raise ValueError if no separator), and preserve tuple inputs
and None as-is before passing to NoopElasticsearch (refer to variables
basic_auth, es_basic_auth and the NoopElasticsearch() yield call).
In `@README.rst`:
- Around line 125-127: Fix the two typos in README.rst by replacing "to
authentication" with "no authentication" and "achived" with "achieved" so the
sentence reads "If none is provided, no authentication is assumed." and "The
same may be achieved by adding options to `pyproject.toml` / `pytest.ini`:";
update the phrases where they appear in the README.rst content.
---
Duplicate comments:
In `@README.rst`:
- Around line 174-185: Update the README.rst auth rows so the "pytest.ini
option" column uses the exact option names registered in plugin.py: replace
occurrences like "api_key" and "basic_auth" with "elasticsearch_api_key" and
"elasticsearch_basic_auth" respectively (and ensure any CLI flag examples like
"--elasticsearch-api-key" map to the full underscore names shown in pytest.ini);
verify the table rows describing the API key and basic auth entries match the
option names defined in plugin.py to avoid mismatch.
- Around line 131-134: Update the TOML header from "[tool.pytest]" to
"[tool.pytest.ini_options]" so pytest recognizes these ini-style settings
(supported since pytest 6.0); leave the keys elasticsearch_host,
elasticsearch_port, and elasticsearch_basic_auth unchanged and ensure the values
remain quoted as shown.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/build.yml.github/workflows/diagram.yml.github/workflows/pypi.yml.pre-commit-config.yamlREADME.rstnewsfragments/869.feature.rstpytest_elasticsearch/config.pypytest_elasticsearch/executor.pypytest_elasticsearch/factories/client.pypytest_elasticsearch/factories/noprocess.pypytest_elasticsearch/plugin.py
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/diagram.yml
- .github/workflows/pypi.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- pytest_elasticsearch/factories/client.py
b171792 to
ba2ce78
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pytest_elasticsearch/factories/noprocess.py (1)
39-49:⚠️ Potential issue | 🟠 Major
basic_authstring args still bypass normalisation.On Line 39 to Line 49, only config-derived values are parsed; a direct
basic_auth="user:password"argument is passed through as a raw string instead of a tuple. This reintroduces the previously flagged inconsistency and can fail at client construction depending on accepted auth types.🐛 Proposed fix
es_basic_auth = basic_auth - if es_basic_auth is None: - es_basic_auth = config.basic_auth - if es_basic_auth is not None: - user, sep, password = es_basic_auth.partition(":") - if not sep: - raise ValueError( - "elasticsearch_basic_auth must be in the format 'username:password'" - ) - es_basic_auth = (user, password) + if es_basic_auth is None: + es_basic_auth = config.basic_auth + if isinstance(es_basic_auth, str): + user, sep, password = es_basic_auth.partition(":") + if not sep: + raise ValueError( + "elasticsearch_basic_auth must be in the format 'username:password'" + ) + es_basic_auth = (user, password)In elasticsearch-py 8.x, what input types are accepted for the `basic_auth` client parameter, and is a single "username:password" string officially supported?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest_elasticsearch/factories/noprocess.py` around lines 39 - 49, The passed-in basic_auth argument is not normalized when it is a "user:pass" string—only config.basic_auth gets parsed—so update the normalization logic for es_basic_auth (the es_basic_auth variable set from basic_auth) to detect when basic_auth is a string containing ":" and split it into (user, password) the same way the config branch does; ensure this happens before falling back to config.basic_auth so both direct basic_auth and config.basic_auth are converted to a (user, password) tuple and ValueError is raised if the separator is missing (referencing the es_basic_auth variable, the basic_auth parameter, and config.basic_auth).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pytest_elasticsearch/factories/noprocess.py`:
- Around line 39-49: The passed-in basic_auth argument is not normalized when it
is a "user:pass" string—only config.basic_auth gets parsed—so update the
normalization logic for es_basic_auth (the es_basic_auth variable set from
basic_auth) to detect when basic_auth is a string containing ":" and split it
into (user, password) the same way the config branch does; ensure this happens
before falling back to config.basic_auth so both direct basic_auth and
config.basic_auth are converted to a (user, password) tuple and ValueError is
raised if the separator is missing (referencing the es_basic_auth variable, the
basic_auth parameter, and config.basic_auth).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.rstnewsfragments/869.feature.rstpytest_elasticsearch/config.pypytest_elasticsearch/executor.pypytest_elasticsearch/factories/noprocess.pypytest_elasticsearch/plugin.py
🚧 Files skipped from review as they are similar to previous changes (3)
- README.rst
- pytest_elasticsearch/plugin.py
- pytest_elasticsearch/executor.py
Signed-off-by: Hans Maerki <buhtig.hans.maerki@ergoinfo.ch>
975c94c to
28e03ee
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pytest_elasticsearch/factories/noprocess.py (1)
39-49:⚠️ Potential issue | 🟠 Major
basic_authstring normalisation is still inconsistent.On Line 39–Line 49, string parsing/validation is applied only to
config.basic_auth. A direct call likeelasticsearch_noproc(basic_auth="user:pass")bypasses conversion and is forwarded as raw string. Normalise and validate both sources identically (including empty-part checks).Proposed fix
- es_basic_auth = basic_auth - if es_basic_auth is None: - es_basic_auth = config.basic_auth - if es_basic_auth is not None: - user, sep, password = es_basic_auth.partition(":") - if not sep: - raise ValueError( - "elasticsearch_basic_auth must be in the format 'username:password'" - ) - es_basic_auth = (user, password) + es_basic_auth = basic_auth if basic_auth is not None else config.basic_auth + if isinstance(es_basic_auth, str): + user, sep, password = es_basic_auth.partition(":") + if not sep or not user or not password: + raise ValueError( + "elasticsearch_basic_auth must be in the format 'username:password'" + ) + es_basic_auth = (user, password)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest_elasticsearch/factories/noprocess.py` around lines 39 - 49, The handling of basic_auth only parses/validates config.basic_auth, so update the normalization to apply the same logic when basic_auth is passed directly: in the block that sets es_basic_auth = basic_auth, detect if es_basic_auth is a str and then partition(":"), require a separator and non-empty username and password (same checks you already have for config.basic_auth), convert it to a (user, password) tuple; if es_basic_auth is None fall back to config.basic_auth and keep the existing parsing/validation, and preserve tuples/other truthy types unchanged; use the existing symbols es_basic_auth, basic_auth, config.basic_auth to locate where to apply this.
🧹 Nitpick comments (1)
pytest_elasticsearch/executor.py (1)
15-23: Update constructor docstring for new parameters.
NoopElasticsearch.__init__now exposes four extra arguments, but the docstring still documents onlyhostandport. Please document the new parameters to keep the public surface self-describing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest_elasticsearch/executor.py` around lines 15 - 23, Update the docstring for NoopElasticsearch.__init__ to document the four new parameters (api_key, basic_auth, request_timeout, verify_certs): describe each parameter name, its expected type (api_key: str | tuple[str, str] | None, basic_auth: str | tuple[str, str] | None, request_timeout: float, verify_certs: bool), default values (api_key=None, basic_auth=None, request_timeout=30, verify_certs=False), and their effect (authentication and request/SSL behavior) so the public constructor signature is fully described; locate the docstring immediately above the __init__ definition to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pytest_elasticsearch/factories/noprocess.py`:
- Around line 39-49: The handling of basic_auth only parses/validates
config.basic_auth, so update the normalization to apply the same logic when
basic_auth is passed directly: in the block that sets es_basic_auth =
basic_auth, detect if es_basic_auth is a str and then partition(":"), require a
separator and non-empty username and password (same checks you already have for
config.basic_auth), convert it to a (user, password) tuple; if es_basic_auth is
None fall back to config.basic_auth and keep the existing parsing/validation,
and preserve tuples/other truthy types unchanged; use the existing symbols
es_basic_auth, basic_auth, config.basic_auth to locate where to apply this.
---
Nitpick comments:
In `@pytest_elasticsearch/executor.py`:
- Around line 15-23: Update the docstring for NoopElasticsearch.__init__ to
document the four new parameters (api_key, basic_auth, request_timeout,
verify_certs): describe each parameter name, its expected type (api_key: str |
tuple[str, str] | None, basic_auth: str | tuple[str, str] | None,
request_timeout: float, verify_certs: bool), default values (api_key=None,
basic_auth=None, request_timeout=30, verify_certs=False), and their effect
(authentication and request/SSL behavior) so the public constructor signature is
fully described; locate the docstring immediately above the __init__ definition
to update.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.rstnewsfragments/869.feature.rstpytest_elasticsearch/config.pypytest_elasticsearch/executor.pypytest_elasticsearch/factories/client.pypytest_elasticsearch/factories/noprocess.pypytest_elasticsearch/plugin.py
🚧 Files skipped from review as they are similar to previous changes (3)
- newsfragments/869.feature.rst
- README.rst
- pytest_elasticsearch/factories/client.py
28e03ee to
48b79f5
Compare
Signed-off-by: Hans Maerki <buhtig.hans.maerki@ergoinfo.ch>
48b79f5 to
0513e1d
Compare
|
@hmaerki Got two questions:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pytest_elasticsearch/factories/noprocess.py (1)
39-49:⚠️ Potential issue | 🟡 MinorHarden
basic_authvalidation for empty and malformed values.Current parsing accepts
":","user:", and":pass"and passes through malformed tuple values unchanged. Please fail fast with strict validation before yielding the fixture.Suggested patch
es_basic_auth = basic_auth if es_basic_auth is None: es_basic_auth = config.basic_auth if isinstance(es_basic_auth, str): user, sep, password = es_basic_auth.partition(":") - if not sep: + if not sep or not user or not password: raise ValueError( - "elasticsearch_basic_auth must be in the format 'username:password'" + "elasticsearch_basic_auth must be in the format 'username:password' with non-empty values" ) es_basic_auth = (user, password) + elif es_basic_auth is not None: + if ( + not isinstance(es_basic_auth, tuple) + or len(es_basic_auth) != 2 + or not all(isinstance(v, str) and v for v in es_basic_auth) + ): + raise ValueError( + "basic_auth must be a tuple of two non-empty strings: (username, password)" + ) es_api_key = api_key or config.api_key🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest_elasticsearch/factories/noprocess.py` around lines 39 - 49, The current es_basic_auth parsing in the fixture (variables: es_basic_auth, basic_auth, config.basic_auth) accepts empty/malformed values like ":", "user:", ":pass" and leaves malformed tuples untouched; update validation so that if es_basic_auth is a str you split once (e.g., split(':',1)) and require both username and password to be non-empty strings, raising ValueError with the same message if either is missing, and if es_basic_auth is a tuple/list ensure it has exactly two elements that are non-empty strings (otherwise raise ValueError); after validation convert a valid string into a (user, password) tuple and reject any other types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pytest_elasticsearch/factories/noprocess.py`:
- Around line 39-49: The current es_basic_auth parsing in the fixture
(variables: es_basic_auth, basic_auth, config.basic_auth) accepts
empty/malformed values like ":", "user:", ":pass" and leaves malformed tuples
untouched; update validation so that if es_basic_auth is a str you split once
(e.g., split(':',1)) and require both username and password to be non-empty
strings, raising ValueError with the same message if either is missing, and if
es_basic_auth is a tuple/list ensure it has exactly two elements that are
non-empty strings (otherwise raise ValueError); after validation convert a valid
string into a (user, password) tuple and reject any other types.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pytest_elasticsearch/executor.pypytest_elasticsearch/factories/client.pypytest_elasticsearch/factories/noprocess.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pytest_elasticsearch/executor.py
- pytest_elasticsearch/factories/client.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
I have never used elastic as a process. $ elasticsearch --help
Starts Elasticsearch
Option Description
------ -----------
-E <KeyValuePair> Configure a setting
-V, --version Prints Elasticsearch version information and exits
-d, --daemonize Starts Elasticsearch in the background
--enrollment-token An existing enrollment token for securely joining a
cluster
-h, --help Show help
-p, --pidfile <Path> Creates a pid file in the specified path on start
-q, --quiet Turns off standard output/error streams logging in console
-s, --silent Show minimal output
-v, --verbose Show verbose output I can not see how to provide authentication. We can use So may answer would be no!
First a question about https://github.com/dbfixtures/pytest-elasticsearch/blob/main/tests/conftest.py#L14. The Second question about https://github.com/dbfixtures/pytest-elasticsearch/blob/main/tests/test_elastic.py#L36-L38: def test_external_elastic(
elasticsearch2: Elasticsearch,
elasticsearch2_noop: Elasticsearch,In this test, Did I understand this correct? Let me think about I would implement a new test for authentication after I got your feedback. |
As for adding tests itself through the noprocess fixture, I think we'd need to start a Docker-based instance and connect the noprocess fixture to it in a test, not through process fixture. |
Add paramters for api_key and basic_auth
This PR allows to provide api_key or basic_auth:
It is also possible to specify it in
pyproject.toml/pytest.ini:Implementation
factories.elasticsearch_noprocTesting
Tested both variants (pyproject.toml / fixture) and basic_auth/api_key against the reference docker-container https://www.elastic.co/docs/deploy-manage/deploy/self-managed/local-development-installation-quickstart.
Notes
This resolves #327.
Summary by CodeRabbit
New Features
Documentation