Tmtee helper driver#641
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded@mangelajo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
WalkthroughAdds project and driver-authoring documentation, updates the driver scaffold to normalize module names, introduces a new jumpstarter-driver-tmt package (driver, client CLI, tests, examples, packaging), enables Rich-based logging and a --log-level CLI option in the composite client, and replaces core client default handler with RichHandler. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CompositeClient CLI
participant TMTc as TMTClient
participant SSH as SSH Child Driver
participant PF as TcpPortforwardAdapter
participant TMT as Local TMT (subprocess)
User->>CLI: jumpstarter ... tmt [options] [args]
Note over CLI: --log-level handled (installs rich.traceback, sets log levels)
CLI->>TMTc: dispatch tmt command (passes click_group if supported)
alt --tmt-on-exporter
TMTc-->>User: not-implemented message
else Local execution
TMTc->>TMTc: get_default_user_pass(), get_reboot_cmd()
alt --forward-ssh
TMTc->>PF: request forwarded host:port via SSH child
PF-->>TMTc: forwarded host:port
TMTc->>TMT: run tmt_cmd with replaced provision args
TMT-->>TMTc: returncode
else Direct address
TMTc->>SSH: address()
alt address supported
SSH-->>TMTc: host:port
TMTc->>TMT: run tmt_cmd with replaced provision args
TMT-->>TMTc: returncode
else not implemented
TMTc->>PF: fallback to port-forward
PF-->>TMTc: forwarded host:port
TMTc->>TMT: run tmt_cmd with replaced provision args
TMT-->>TMTc: returncode
end
end
TMTc-->>User: exit code
end
sequenceDiagram
autonumber
actor User
participant CCLI as CompositeClient CLI
participant CB as Click Base Group
participant Child as Child Client
User->>CCLI: run composite command
CCLI->>CCLI: --log-level callback sets logger levels + rich.traceback
loop register child commands
CCLI->>Child: call cli(click_group=CB) if accepts it
alt supports click_group
Child-->>CCLI: registers into parent group
else
Child-->>CCLI: returns standalone command (added)
end
end
CCLI-->>User: commands available with unified log level
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mangelajo
force-pushed
the
tmtee-helper-driver
branch
from
142d899 to
154a5b8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/jumpstarter/jumpstarter/client/core.py (1)
165-169: Bug: passing a string log level to logger.log() (expects int).logging.getLevelName(int) returns a string like "DEBUG"; logger.log requires an int level. This will mis-log or fail.
- self.__log(logging.getLevelName(response.severity), response.message) + # severity is expected to be a numeric logging level + self.__log(int(response.severity), response.message)__templates__/create_driver.sh (1)
26-33: Bug: gsed path does not include -i; use arrays for sed command.On systems with gsed present, in-place edit won’t happen. Using an array handles macOS empty arg safely.
-if command -v gsed &> /dev/null; then - sed_cmd="gsed" -elif [[ "$(uname)" == "Darwin" ]]; then - sed_cmd="sed -i ''" -else - sed_cmd="sed -i" -fi +if command -v gsed &>/dev/null; then + sed_cmd=(gsed -i) +elif [[ "$(uname)" == "Darwin" ]]; then + sed_cmd=(sed -i '') +else + sed_cmd=(sed -i) +fi
🧹 Nitpick comments (21)
packages/jumpstarter-driver-tmt/.gitignore (1)
1-3: Consider expanding ignores to common Python artifacts.Recommend adding .pytest_cache, htmlcov/, .mypy_cache/, .ruff_cache/, .venv/, build/, dist/, *.egg-info for consistency across packages.
__pycache__/ .coverage coverage.xml +htmlcov/ +.pytest_cache/ +.mypy_cache/ +.ruff_cache/ +*.egg-info +build/ +dist/ +.venv/packages/jumpstarter/jumpstarter/client/core.py (1)
74-77: Avoid duplicate logs; normalize level.
- If root already has handlers, adding one here may duplicate messages via propagation.
- setLevel() with a lowercase string raises ValueError.
- self.logger.setLevel(self.log_level) + self.logger.setLevel(self.log_level.upper() if isinstance(self.log_level, str) else self.log_level) ... - if not self.logger.handlers: - handler = RichHandler() - self.logger.addHandler(handler) + if not self.logger.handlers: + handler = RichHandler() + self.logger.addHandler(handler) + self.logger.propagate = False # prevent double logging if root is configuredpackages/jumpstarter-driver-tmt/pyproject.toml (1)
4-4: Replace placeholder description.Update description to something meaningful for packaging and docs.
-description = "Add your description here" +description = "Jumpstarter TMT driver to run TMT locally with SSH/DUT integration"packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (2)
11-26: Log setup is heavy-handed and may suppress/override library logger intent.
- Forcing every existing logger’s level can be surprising; configuring only the root is standard.
- No handler is configured; raising the level alone may still produce no output.
- Duplicates functionality that already exists in jumpstarter-cli-common.
-from rich import traceback +from rich import traceback +from rich.logging import RichHandler +from functools import partial ... -def _opt_log_level_callback(ctx, param, value): - traceback.install() - - # Set the log level - log_level = value.upper() if value else "INFO" - - # Update the root logger level to ensure all loggers inherit it - root_logger = logging.getLogger() - root_logger.setLevel(log_level) - - # Update all existing loggers to use the new level - for logger_name in logging.Logger.manager.loggerDict: - logger = logging.getLogger(logger_name) - logger.setLevel(log_level) +def _opt_log_level_callback(ctx, param, value): + traceback.install() + basicConfig = partial(logging.basicConfig, handlers=[RichHandler()]) + basicConfig(level=(value.upper() if value else logging.INFO))Alternatively, import and reuse the shared helper:
-from rich import traceback +from jumpstarter_cli_common.opt import _opt_log_level_callbackIf adopting the shared helper, ensure jumpstarter-driver-composite depends on jumpstarter-cli-common in its pyproject.toml.
51-58: Child CLI registration: good flexibility, but guard for callables returning None.If a child cli() unexpectedly returns None, add_command will raise. Consider a small guard.
- if "click_group" in sig.parameters: - base.add_command(v.cli(click_group=base), k) - else: - base.add_command(v.cli(), k) + cmd = v.cli(click_group=base) if "click_group" in sig.parameters else v.cli() + if cmd is not None: + base.add_command(cmd, k)__templates__/create_driver.sh (2)
23-25: SC2155: assign in two steps; also safer without export if not needed.Avoid masking return values and keep env clean.
-# Convert hyphens to underscores for Python module name -export DRIVER_MODULE_NAME=$(echo "${DRIVER_NAME}" | sed 's/-/_/g') +# Convert hyphens to underscores for Python module name +DRIVER_MODULE_NAME="$(printf '%s' "${DRIVER_NAME}" | sed 's/-/_/g')" +export DRIVER_MODULE_NAME
47-71: In-place README substitutions: invoke sed array; minor hardening.Use the array form and avoid unquoted var expansions in sed script values.
-$sed_cmd "s/\${DRIVER_CLASS}/${DRIVER_CLASS}/g; s/\${DRIVER_NAME}/${DRIVER_NAME}/g; s/\${DRIVER_MODULE_NAME}/${DRIVER_MODULE_NAME}/g" "${README_FILE}" +"${sed_cmd[@]}" "s|\${DRIVER_CLASS}|${DRIVER_CLASS}|g; s|\${DRIVER_NAME}|${DRIVER_NAME}|g; s|\${DRIVER_MODULE_NAME}|${DRIVER_MODULE_NAME}|g" "${README_FILE}"Optional: drop
-vfromset -euxvto reduce noisy output.Also applies to: 77-77
.cursor/rules/creating-new-drivers.mdc (1)
106-108: Typo: “Drives” → “Drivers”.Small wording fix.
-Drives which have children drivers should be composite drivers, and the client interface should +Drivers which have children drivers should be composite drivers, and the client interface shoulddocs/source/reference/package-apis/drivers/tmt.md (3)
1-1: Remove stray path line.The raw path line will render as text; the content below already documents the driver.
-../../../../../packages/jumpstarter-driver-tmt/README.md
67-69: Fix broken line wraps in the example command.The command is split mid-word; keep it on one line or use backslashes.
-j --log-level DEBUG tmt --root . -c tracing=off -c arch=aarch64 -c distro=rhel-9 -c hw_target=r -car_s4 run --workdir-root /tmp/ -a -vv provision .. some other provisioning... plan -vv --name ^/podman/plans/fusa/te -sts$ +j --log-level DEBUG tmt --root . -c tracing=off -c arch=aarch64 -c distro=rhel-9 -c hw_target=rcar_s4 run --workdir-root /tmp/ -a -vv provision .. some other provisioning... plan -vv --name '^/podman/plans/fusa/tests$'
88-91: Avoid documenting plaintext passwords in CLI history.Suggest env var or prompt-based input to reduce exposure.
docs/source/contributing.md (1)
52-66: Clarify license policy for AI-generated code.This doc mentions licensing diligence but doesn’t state the repo’s “Apache-2.0 only” policy (documented elsewhere). Add it here to avoid ambiguity for AI-assisted contributions.
Apply this diff:
-This project accepts contributions from AI assistants, although you should be careful when creating code from AI assistants, +This project accepts contributions from AI assistants, although you should be careful when creating code from AI assistants, and figure out if the code you are submitting could infringe any licensing, for example, reusing code from other incompatible -GPL licenses, you should do your due diligence. +GPL licenses, you should do your due diligence. Note: this repository only accepts contributions under the Apache-2.0 license.packages/jumpstarter-driver-tmt/README.md (1)
67-69: Fix broken command wrapping in example.The example is split mid-token, which makes it unusable when copy-pasted.
Apply this diff to keep it on a single line:
-j --log-level DEBUG tmt --root . -c tracing=off -c arch=aarch64 -c distro=rhel-9 -c hw_target=r -car_s4 run --workdir-root /tmp/ -a -vv provision .. some other provisioning... plan -vv --name ^/podman/plans/fusa/te -sts$ +j --log-level DEBUG tmt --root . -c tracing=off -c arch=aarch64 -c distro=rhel-9 -c hw_target=rcar_s4 run --workdir-root /tmp/ -a -vv provision .. some other provisioning... plan -vv --name ^/podman/plans/fusa/tests$.cursor/rules/project-structure.mdc (1)
128-136: Add guidance on handling secrets in configs.Key conventions omit secret management; given drivers accept credentials, add a note to avoid plaintext secrets in repo configs.
Apply this diff:
6. **Dependencies**: Minimal, focused dependencies per package +7. **Secrets**: Do not commit plaintext credentials in examples; prefer environment variables, secret stores, or redacted placeholders.packages/jumpstarter-driver-tmt/examples/exporter.yaml (2)
11-15: Avoid plaintext passwords in examples.Use a placeholder and a comment pointing to a secret source to prevent copy‑pasting real creds into repos.
Apply this diff:
- default_password: "somePass" + # Avoid plaintext in real configs. Use a secret store or env var. + default_password: "<password>"
13-14: Keep examples consistent with README.README uses “somePassword”; this file uses “somePass”. Unify to a single placeholder to reduce confusion.
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py (1)
27-33: Add return type hints to exported methods.Improves API clarity and downstream typing in clients/tests.
Apply this diff:
- def get_reboot_cmd(self): + def get_reboot_cmd(self) -> str: return self.reboot_cmd - def get_default_user_pass(self): + def get_default_user_pass(self) -> tuple[str, str]: return self.default_username, self.default_passwordpackages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (3)
29-40: Invoke the Click command correctly.You’re passing “tmt” as an argument to the tmt command. When testing the command itself (not the group), call with its args only.
Apply this diff:
- result = runner.invoke(cli, ["tmt"]) + result = runner.invoke(cli, [])
68-72: Same issue: remove redundant subcommand token.Apply this diff:
- result = runner.invoke(cli, ["tmt", "--tmt-cmd", "custom-tmt", "test"]) + result = runner.invoke(cli, ["--tmt-cmd", "custom-tmt", "test"])
156-173: Add fallback test when ssh.address() raises ValueError.TcpNetwork.address raises ValueError when enable_address is false; client should fall back to forwarding in that case too.
Apply this new test at the end of the file:
def test_drivers_tmt_run_tmt_fallback_on_value_error(): instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)}) with serve(instance) as client: client.ssh.address = MagicMock(side_effect=ValueError("enable_address mode is not true")) with patch('jumpstarter_driver_tmt.client.TcpPortforwardAdapter') as mock_adapter: mock_adapter.return_value.__enter__.return_value = ("localhost", 2222) with patch.object(client, '_run_tmt_local') as mock_run_tmt: mock_run_tmt.return_value = 0 result = client.run_tmt_local(False, "tmt", "user", "pass", ["arg1"]) assert result == 0 mock_run_tmt.assert_called_once_with( "localhost", 2222, "tmt", "user", "pass", "j power cycle", ["arg1"] )packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
12-18: Docstring naming nit.References “LocalTMT driver” while the class is TMTClient. Update for consistency.
Apply this diff:
- Client interface for LocalTMT driver + Client interface for TMT driver
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.cursor/rules/creating-new-drivers.mdc(1 hunks).cursor/rules/project-structure.mdc(1 hunks)__templates__/create_driver.sh(5 hunks)docs/source/contributing.md(1 hunks)docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/tmt.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(2 hunks)packages/jumpstarter-driver-tmt/.gitignore(1 hunks)packages/jumpstarter-driver-tmt/README.md(1 hunks)packages/jumpstarter-driver-tmt/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py(1 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py(1 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py(1 hunks)packages/jumpstarter-driver-tmt/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/client/core.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Applied to files:
packages/jumpstarter-driver-tmt/README.mdpackages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.pydocs/source/reference/package-apis/drivers/tmt.mdpackages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py
📚 Learning: 2025-02-08T16:08:34.616Z
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#267
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py:32-40
Timestamp: 2025-02-08T16:08:34.616Z
Learning: In the TFTP driver (jumpstarter_driver_tftp), the TFTP server component is read-only, while file management operations (upload, delete) are handled through the driver's client interface, not through the TFTP protocol itself.
Applied to files:
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py
🧬 Code graph analysis (4)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py (1)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-269)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (2)
_opt_log_level_callback(10-17)callback(217-218)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (6)
base(406-407)base(541-543)cli(397-511)cli(539-579)cli(706-730)cli(780-782)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
cli(20-41)packages/jumpstarter-cli/jumpstarter_cli/j.py (1)
cli(18-22)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (4)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py (1)
client(24-25)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (5)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py (2)
client(24-25)TMT(8-33)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (4)
replace_provision_args(90-128)cli(20-41)run_tmt_local(43-69)_run_tmt_local(71-78)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
🪛 Shellcheck (0.11.0)
__templates__/create_driver.sh
[warning] 24-24: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (4)
docs/source/reference/package-apis/drivers/index.md (1)
82-83: Docs entry LGTM.TMT added consistently to list and toctree.
Also applies to: 106-106
packages/jumpstarter/jumpstarter/client/core.py (1)
15-15: No action required —richis already declared.
packages/jumpstarter/pyproject.toml lists "rich>=14.0.0"; RichHandler imports are present in packages/jumpstarter and packages/jumpstarter-cli-common.packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py (1)
19-21: Good config validation.Requiring an “ssh” child early prevents misconfiguration at runtime.
packages/jumpstarter-driver-tmt/README.md (1)
7-9: Confirm package is published to pkg.jumpstarter.dev/simple or update README.
- packages/jumpstarter-driver-tmt/pyproject.toml — name = "jumpstarter-driver-tmt" (verified).
- No public listing for jumpstarter-driver-tmt was found on the index; publish the package to https://pkg.jumpstarter.dev/simple or update the README. Verify with: curl -I https://pkg.jumpstarter.dev/simple/jumpstarter-driver-tmt/ or pip index versions --extra-index-url https://pkg.jumpstarter.dev/simple jumpstarter-driver-tmt. (jumpstarter.dev)
mangelajo
force-pushed
the
tmtee-helper-driver
branch
from
154a5b8 to
f6dcce9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
51-58: Child CLI wiring is pragmatic; consider broader param name supportYou match on a parameter literally named click_group. Some existing drivers use base or group. Consider accepting any single optional parameter for the click group to improve compatibility.
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (2)
131-139: Adjust mocks for async context manager (aenter)TcpPortforwardAdapter is an async context manager; mock aenter instead of enter.
- with patch('jumpstarter_driver_tmt.client.TcpPortforwardAdapter') as mock_adapter: - mock_adapter.return_value.__enter__.return_value = ("localhost", 2222) + with patch('jumpstarter_driver_tmt.client.TcpPortforwardAdapter') as mock_adapter: + mock_adapter.return_value.__aenter__.return_value = ("localhost", 2222)
114-116: If run_tmt_local becomes async, wrap calls with asyncio.run in testsAfter fixing the async context manager usage, update direct invocations accordingly.
Example:
- result = client.run_tmt_local(False, "tmt", None, None, []) + import asyncio + result = asyncio.run(client.run_tmt_local(False, "tmt", None, None, [])) - result = client.run_tmt_local(True, "custom-tmt", "user", "pass", ["arg1", "arg2"]) + result = asyncio.run(client.run_tmt_local(True, "custom-tmt", "user", "pass", ["arg1", "arg2"])) - result = client.run_tmt_local(True, "tmt", "user", "pass", ["arg1"]) + result = asyncio.run(client.run_tmt_local(True, "tmt", "user", "pass", ["arg1"])) - result = client.run_tmt_local(False, "tmt", "user", "pass", ["arg1"]) + result = asyncio.run(client.run_tmt_local(False, "tmt", "user", "pass", ["arg1"])) - result = client.run_tmt_local(False, "tmt", "user", "pass", ["arg1"]) + result = asyncio.run(client.run_tmt_local(False, "tmt", "user", "pass", ["arg1"]))Also applies to: 121-123, 135-139, 149-153, 169-173
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (2)
12-24: Redact more robustly (multiple occurrences, long flags)Current logic redacts only the first -p. Support repeated and long-form flags.
-def redact_password_in_args(args): - """Redact password arguments in a list for safe logging""" - redacted = list(args) - try: - # Find -p flag and redact the next argument (password) - p_index = redacted.index("-p") - if p_index + 1 < len(redacted): - redacted[p_index + 1] = "******" - except ValueError: - # -p flag not found, nothing to redact - pass - return redacted +def redact_password_in_args(args): + """Redact password arguments in a list for safe logging""" + out = [] + i = 0 + while i < len(args): + tok = args[i] + out.append(tok) + if tok in ("-p", "--password") and i + 1 < len(args): + out.append("******") + i += 2 + continue + i += 1 + return out
87-103: Subprocess call is safe; consider check=True and richer error messaging laterNon-blocking note: optional improvement for propagation. No action needed now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.cursor/rules/project-structure.mdc(1 hunks)docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/tmt.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(2 hunks)packages/jumpstarter-driver-composite/pyproject.toml(1 hunks)packages/jumpstarter-driver-tmt/.gitignore(1 hunks)packages/jumpstarter-driver-tmt/README.md(1 hunks)packages/jumpstarter-driver-tmt/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py(1 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py(1 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py(1 hunks)packages/jumpstarter-driver-tmt/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/client/core.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/jumpstarter-driver-composite/pyproject.toml
- packages/jumpstarter-driver-tmt/README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/jumpstarter-driver-tmt/.gitignore
- docs/source/reference/package-apis/drivers/tmt.md
- .cursor/rules/project-structure.mdc
- packages/jumpstarter/jumpstarter/client/core.py
- docs/source/reference/package-apis/drivers/index.md
- packages/jumpstarter-driver-tmt/examples/exporter.yaml
- packages/jumpstarter-driver-tmt/pyproject.toml
- packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (5)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py (1)
client(24-25)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (2)
CompositeClient(28-60)cli(37-60)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (6)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py (2)
client(24-25)TMT(8-33)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (4)
replace_provision_args(114-156)cli(34-55)run_tmt_local(57-85)_run_tmt_local(87-102)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
cli(37-60)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (5)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver.py (2)
client(11-12)client(25-26)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (6)
base(406-407)base(541-543)cli(397-511)cli(539-579)cli(706-730)cli(780-782)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
cli(34-55)packages/jumpstarter-cli/jumpstarter_cli/j.py (1)
cli(18-22)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
cli(19-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (2)
11-26: Log-level callback: avoid clobbering all third‑party loggersSetting every existing logger’s level can override library defaults and spam output. Limit changes to root and our namespaces; also iterate over a snapshot to avoid “dict changed size” risks.
[ suggest_recommended_refactor ]Apply:
def _opt_log_level_callback(ctx, param, value): traceback.install() # Set the log level log_level = value.upper() if value else "INFO" - # Update the root logger level to ensure all loggers inherit it + # Update the root logger level to ensure our loggers inherit it root_logger = logging.getLogger() root_logger.setLevel(log_level) - # Update all existing loggers to use the new level - for logger_name in logging.Logger.manager.loggerDict: - logger = logging.getLogger(logger_name) - logger.setLevel(log_level) + # Update only our loggers, avoid touching 3rd-party ones + for logger_name in list(logging.Logger.manager.loggerDict.keys()): + if logger_name.startswith(("jumpstarter", "jumpstarter_driver_")): + logging.getLogger(logger_name).setLevel(log_level)
37-47: Nice: global --log-level option wired at top groupClean way to initialize Rich tracebacks and log levels before subcommands run.
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (1)
165-173: Same: use aenter for the async adapter in fallback testMirror the fix here.
- with patch('jumpstarter_driver_tmt.client.TcpPortforwardAdapter') as mock_adapter: - mock_adapter.return_value.__enter__.return_value = ("localhost", 2222) + with patch('jumpstarter_driver_tmt.client.TcpPortforwardAdapter') as mock_adapter: + mock_adapter.return_value.__aenter__.return_value = ("localhost", 2222)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (2)
83-86: Good: fallback catches ValueError tooCovers TcpNetwork.address() disabled mode.
If any other ssh drivers raise different exceptions on address() failure, consider broadening or normalizing in a shared helper.
57-86: Use async with for TcpPortforwardAdapter (it’s an async context manager)Current sync with will fail at runtime. Make run_tmt_local async and drive it from the CLI via asyncio.run.
- def run_tmt_local(self, forward_ssh, tmt_cmd, username, password, args): + async def run_tmt_local(self, forward_ssh, tmt_cmd, username, password, args): @@ - if forward_ssh: + if forward_ssh: self.logger.debug("Using SSH port forwarding for TMT connection") - with TcpPortforwardAdapter( - client=self.ssh, - ) as addr: + async with TcpPortforwardAdapter( + client=self.ssh, + ) as addr: host = addr[0] port = addr[1] self.logger.debug(f"SSH port forward established - host: {host}, port: {port}") return self._run_tmt_local(host, port, tmt_cmd, username, password, hard_reboot_cmd, args) @@ - except (DriverMethodNotImplemented, ValueError) as e: + except (DriverMethodNotImplemented, ValueError) as e: self.logger.warning(f"Direct address connection failed ({e}), falling back to SSH port forwarding") - return self.run_tmt_local(True, tmt_cmd, username, password, args) + return await self.run_tmt_local(True, tmt_cmd, username, password, args)And drive it from the CLI:
- result = self.run_tmt_local(forward_ssh, tmt_cmd, tmt_username, tmt_password, args) + import asyncio + result = asyncio.run(self.run_tmt_local(forward_ssh, tmt_cmd, tmt_username, tmt_password, args))If not already imported, add near the top:
+import asyncio
For example: j command instead of: j command subcommand The code now detects if the cli implementation of the client receives click_group and passes it down, letting the client driver decide the structure
mangelajo
force-pushed
the
tmtee-helper-driver
branch
from
f6dcce9 to
cc12051
Compare
|
Moved several independent details to their own PRs |
|
LGTM |
@kirkbrauer we have benny/bella/etc out, can you give me a +1 to merge this driver? |
| from jumpstarter.client import DriverClient | ||
|
|
||
|
|
||
| def _opt_log_level_callback(ctx, param, value): |
There was a problem hiding this comment.
Why can't we just import this from jumpstarter_cli_common?
| for k, v in self.children.items(): | ||
| if hasattr(v, "cli"): | ||
| base.add_command(v.cli(), k) | ||
| # Check if the cli method accepts a click_group parameter |
There was a problem hiding this comment.
What does click_group do?
There was a problem hiding this comment.
It's used to setup a top level command without subcommands in j
|
Sorry Nick, I clicked merge without seeing your comments. |
|
Successfully created backport PR for |
4 participants
Adds:
Summary by CodeRabbit
New Features
Documentation
Style
Tests