Skip to content

fix(cli): add config_section and imports#3866

Open
jaegeral wants to merge 25 commits into
google:masterfrom
jaegeral:2026-06-25-cli-client
Open

fix(cli): add config_section and imports#3866
jaegeral wants to merge 25 commits into
google:masterfrom
jaegeral:2026-06-25-cli-client

Conversation

@jaegeral

@jaegeral jaegeral commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Fixing the cli-client to add config_section and 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

@jaegeral

Copy link
Copy Markdown
Collaborator Author

/gemini review

@jaegeral jaegeral self-assigned this Jun 25, 2026
@jaegeral jaegeral added CLI Client Code Health Code health improvements labels Jun 25, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli_client/python/timesketch_cli_client/cli.py Outdated
Comment thread cli_client/python/timesketch_cli_client/cli.py Outdated
@jaegeral

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli_client/python/timesketch_cli_client/commands/config.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/config.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/config.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/config.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/config.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/timelines.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/timelines.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/attribute.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/attribute.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/attribute.py Outdated
@jaegeral
jaegeral marked this pull request as ready for review June 25, 2026 08:14

@jkppr jkppr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of review

Comment thread cli_client/python/timesketch_cli_client/commands/events.py
Comment thread cli_client/python/timesketch_cli_client/commands/config.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/events.py
Comment thread cli_client/python/timesketch_cli_client/commands/events.py
Comment thread cli_client/python/timesketch_cli_client/commands/search_test.py Outdated
Comment thread cli_client/python/timesketch_cli_client/cli.py Outdated
jaegeral and others added 3 commits June 25, 2026 13:13
@jaegeral

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli_client/python/timesketch_cli_client/commands/config_test.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/config_test.py
Comment thread cli_client/python/timesketch_cli_client/cli.py Outdated
Comment thread cli_client/python/timesketch_cli_client/cli.py Outdated
jaegeral and others added 5 commits June 25, 2026 13:36
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>
@jaegeral
jaegeral requested a review from jkppr June 25, 2026 11:55
Comment thread cli_client/python/timesketch_cli_client/commands/sketch.py Outdated
Comment thread cli_client/python/timesketch_cli_client/commands/sketch.py
@jaegeral
jaegeral requested a review from jkppr June 26, 2026 07:52
@jaegeral

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli_client/python/timesketch_cli_client/cli.py
Comment thread cli_client/python/timesketch_cli_client/cli.py
Comment thread cli_client/python/timesketch_cli_client/cli.py
jaegeral and others added 5 commits June 26, 2026 10:10
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>
Comment on lines +91 to +94
click.echo(
"WARNING: The installed timesketch-api-client does not "
"support custom config sections. Falling back to default."
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLI Client Code Health Code health improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants