fix(cli): add config_section and imports#3866
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors relative imports to absolute imports across several CLI client files and introduces a config_section parameter to TimesketchCli to allow specifying a custom configuration section. Feedback on these changes suggests exposing this new parameter as a command-line option in the CLI entry point so users can actually utilize it, and adding unit tests to verify that custom configuration sections are handled correctly.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Timesketch CLI client to use absolute imports, adds a new --config-section option to allow loading custom configuration sections, and updates docstrings across multiple command files. The review feedback highlights several inconsistencies in the updated docstrings, specifically pointing out incorrect type annotations (such as using object instead of specific types like str, int, or bool) and the non-standard addition of (object) to descriptive output types like JSON, Text, and File in the Outputs and Raises sections.
Co-authored-by: Janosch <99879757+jkppr@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for a custom configuration section (--config-section) in the Timesketch CLI client, allowing users to specify different configuration profiles. It also cleans up docstrings and updates relative imports to absolute imports across several command and test files. The review feedback highlights several critical improvements: wrapping inspect.signature calls in try...except blocks to prevent crashes when functions are mocked or uninspectable, using autospec=True on mocked functions in tests so that signature inspection works correctly, and adding assertions to verify that the custom configuration section is properly propagated to the API client.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for custom configuration sections in the Timesketch CLI client by introducing a new --config-section option and dynamically checking API client compatibility. It also cleans up imports and docstrings across various command modules. The review feedback suggests separating standard library and third-party imports in cli.py to comply with the style guide, and adding warning messages to notify users if their requested custom configuration section is silently ignored due to an outdated API client.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| click.echo( | ||
| "WARNING: The installed timesketch-api-client does not " | ||
| "support custom config sections. Falling back to default." | ||
| ) |
There was a problem hiding this comment.
Falling back to the default config section if the requested one isn't supported seems a bit risky to me. If a user explicitly asks for a specific section (like prod) and it silently falls back to default (which might be dev), they could run commands on the wrong server.
I suggest we should fail and exit with an error here instead of warning and continuing.
| config_path=conf_file, load_cli_config=True | ||
| ) | ||
| try: | ||
| sig = inspect.signature(timesketch_config.get_client) |
There was a problem hiding this comment.
Using inspect.signature to check for dependency features at runtime feels a bit fragile. Since both the CLI and API clients live in this same repo, can we just bump the required version of timesketch-api-client in cli_client/python/setup.py instead? That way we can assume the feature is present and clean up this check.
Fixing the cli-client to add
config_sectionand improving various imports.I've updated the ignored-argument-names configuration inside .pylintrc by appending |^ctx$ to it to ignore docstring checks in
cli.py