Skip to content
This repository was archived by the owner on Jan 23, 2026. It is now read-only.

don't add to group#684

Merged
mangelajo merged 1 commit intojumpstarter-dev:mainfrom
michalskrivanek:ssh-group
Oct 3, 2025
Merged

don't add to group#684
mangelajo merged 1 commit intojumpstarter-dev:mainfrom
michalskrivanek:ssh-group

Conversation

@michalskrivanek
Copy link
Copy Markdown
Contributor

@michalskrivanek michalskrivanek commented Oct 3, 2025

SSHWrapper should just define "ssh" method and that's it. Do not modify parent group.
Fixes-Issue: #679

Summary by CodeRabbit

  • New Features

    • SSH and TMT drivers now provide standalone CLI commands for direct invocation.
  • Chores

    • CLI wiring standardized across drivers for consistent behavior.
    • Subcommand registration simplified; invocation may differ if previously used as nested subcommands.
  • Tests

    • Test suites updated to reflect the new flat CLI shape and argument patterns.

@netlify
Copy link
Copy Markdown

netlify Bot commented Oct 3, 2025

Deploy Preview for jumpstarter-docs ready!

Name Link
🔨 Latest commit ff05456
🔍 Latest deploy log https://app.netlify.com/projects/jumpstarter-docs/deploys/68dfd92d2ba4e40008541fe7
😎 Deploy Preview https://deploy-preview-684--jumpstarter-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 3, 2025

Warning

Rate limit exceeded

@michalskrivanek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 19 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 360864e and ff05456.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1 hunks)
  • packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1 hunks)
  • packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1 hunks)
  • packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (5 hunks)

Walkthrough

Removed click_group parameters from several client CLI methods and switched their decorators to standalone @click.command; simplified composite client to always call child v.cli() without inspecting signatures; updated TMT tests to invoke the flattened CLI shape.

Changes

Cohort / File(s) Summary
SSH CLI registration
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
SSHWrapperClient.cli signature changed from cli(self, click_group)cli(self); decorator changed from @click_group.command(...)@click.command(...); internal command logic preserved.
TMT CLI registration & tests
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py, packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py
TMTClient.cli signature changed from cli(self, click_group)cli(self) and decorator switched to @click.command(...). Tests updated to call client.cli() and to invoke the flattened CLI (removed "tmt" subcommand from runner.invoke argument lists).
Composite client simplification
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py
Removed dynamic signature inspection and always call child v.cli() without arguments when registering subcommands; removed unused inspect import.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant ClickCLI as Click
  participant Driver as SSH/TMT Client

  rect rgba(200,230,255,0.25)
  note over ClickCLI: Standalone command registration
  User->>ClickCLI: invoke "ssh" / "tmt" (flat CLI)
  ClickCLI->>Driver: call cli()
  Driver->>Driver: execute command logic
  Driver-->>User: produce output
  end
Loading
sequenceDiagram
  autonumber
  actor Orchestrator
  participant Composite as CompositeClient
  participant Child as ChildClient

  rect rgba(220,255,220,0.25)
  note over Composite: Simplified child registration flow
  Orchestrator->>Composite: register children
  Composite->>Child: call v.cli() (no args)
  Child->>Composite: returns Command
  Composite-->>Orchestrator: attach subcommand
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I hop from branch to branch with glee,
A click, a command — now simple and free.
No groups to chase, no signatures tight,
I bounce through code in morning light.
Logs trail like carrots, crisp and bright. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title “don’t add to group” is too vague and does not clearly convey that the primary change is the removal of CLI command registration from the parent click group for SSH and TMT drivers. It fails to specify what is being removed or which component is affected, making it difficult for other developers to understand the intent at a glance. A more descriptive title would improve clarity and context. Please revise the title to explicitly reference the removal of click group registration, for example “Remove parent click group registration from SSHWrapperClient.cli” or “Unregister SSH and TMT CLI commands from parent click group” so that the main change is clear.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@michalskrivanek
Copy link
Copy Markdown
Contributor Author

@mangelajo I don't entirely follow the logic of how the drivers stack up, but it seems to me the driver should not modify the parent on its own. Composite driver then does it when it should show up under same level as something else.

@mangelajo
Copy link
Copy Markdown
Member

@mangelajo I don't entirely follow the logic of how the drivers stack up, but it seems to me the driver should not modify the parent on its own. Composite driver then does it when it should show up under same level as something else.

The idea is that we didn't want to register subcommands , and put it on the level of the parent, I am going to download and play around.

@mangelajo mangelajo added this to the 0.7.2 milestone Oct 3, 2025
@mangelajo mangelajo linked an issue Oct 3, 2025 that may be closed by this pull request
@mangelajo mangelajo self-requested a review October 3, 2025 08:16
Copy link
Copy Markdown
Member

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

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

Ok, your patch work, and makes sense now that I read it more carefully.

What does not make sense is what I did here jumpstarter-dev/jumpstarter@b99802b just for this driver.

Can you revert the patch as part of your PR ?

@mangelajo
Copy link
Copy Markdown
Member

oh... it was used on tmt too.. so same change is necessary on tmt as you saw on the other PR '':D

sorry I should have done the rest of the cleanup myself on another PR

@mangelajo
Copy link
Copy Markdown
Member

@coderabbitai, can you make a committable suggestion to also remove the click_group reception on the tmt driver client cli?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 3, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current state of the tmt driver client file
cat packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py

Length of output: 7240


@mangelajo, the change you requested has already been made in this PR!

Looking at packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py, the TMTClient.cli method has already been updated:

  1. ✅ Method signature changed from def cli(self, click_group) to def cli(self) (line 29)
  2. ✅ Decorator changed from @click_group.command(...) to @click.command(...) (line 31)

The TMT driver now follows the same pattern as the SSH driver - both have been updated to not receive or modify the parent click group, which aligns with the PR's objective.

@mangelajo
Copy link
Copy Markdown
Member

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain
@mangelajo, the change you requested has already been made in this PR!

Looking at packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py, the TMTClient.cli method has already been updated:

  1. ✅ Method signature changed from def cli(self, click_group) to def cli(self) (line 29)
  2. ✅ Decorator changed from @click_group.command(...) to @click.command(...) (line 31)

The TMT driver now follows the same pattern as the SSH driver - both have been updated to not receive or modify the parent click group, which aligns with the PR's objective.

oh right,

can you update unit tests to handle that too?

=================================== FAILURES ===================================
_____________________________ test_drivers_tmt_cli _____________________________

def test_drivers_tmt_cli():
    """Test the CLI functionality with tmt command and arguments"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        # Test the CLI tmt command without arguments
        runner = CliRunner()
      cli = client.cli(click.Group())
              ^^^^^^^^^^^^^^^^^^^^^^^^^

E TypeError: TMTClient.cli() takes 1 positional argument but 2 were given

jumpstarter_driver_tmt/driver_test.py:27: TypeError
______________________ test_drivers_tmt_cli_with_options _______________________

def test_drivers_tmt_cli_with_options():
    """Test the CLI functionality with various options"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        runner = CliRunner()
      cli = client.cli(click.Group())
              ^^^^^^^^^^^^^^^^^^^^^^^^^

E TypeError: TMTClient.cli() takes 1 positional argument but 2 were given

jumpstarter_driver_tmt/driver_test.py:49: TypeError
_____________________ test_drivers_tmt_cli_error_handling ______________________

def test_drivers_tmt_cli_error_handling():
    """Test CLI error handling when TMT command fails"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        runner = CliRunner()
      cli = client.cli(click.Group())
              ^^^^^^^^^^^^^^^^^^^^^^^^^

E TypeError: TMTClient.cli() takes 1 positional argument but 2 were given

jumpstarter_driver_tmt/driver_test.py:81: TypeError
_____________________ test_drivers_tmt_cli_tmt_on_exporter _____________________

def test_drivers_tmt_cli_tmt_on_exporter():
    """Test CLI with --tmt-on-exporter flag"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        runner = CliRunner()
      cli = client.cli(click.Group())
              ^^^^^^^^^^^^^^^^^^^^^^^^^

E TypeError: TMTClient.cli() takes 1 positional argument but 2 were given

jumpstarter_driver_tmt/driver_test.py:97: TypeError
_________________________ test_drivers_tmt_cli_logging _________________________

def test_drivers_tmt_cli_logging():
    """Test logging in CLI functionality"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        runner = CliRunner()
      cli = client.cli(click.Group())
              ^^^^^^^^^^^^^^^^^^^^^^^^^

E TypeError: TMTClient.cli() takes 1 positional argument but 2 were given

jumpstarter_driver_tmt/driver_test.py:431: TypeError

@mangelajo
Copy link
Copy Markdown
Member

@coderabbitai can you make a commitable suggestion to fix the tmt unit tests?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 3, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the TMT driver test file to see the full context
cat packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py

Length of output: 18587


@mangelajo, here's the fix for the TMT unit tests. The issue is that all the test functions are calling client.cli(click.Group()) with an argument, but the method signature has been changed to take no arguments.

Here's the corrected test file:

from unittest.mock import MagicMock, patch

import click
import pytest
from click.testing import CliRunner
from jumpstarter_driver_network.driver import TcpNetwork

from .client import replace_provision_args
from .driver import TMT
from jumpstarter.common.utils import serve


def test_drivers_tmt():
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        assert client.ssh.address() == "tcp://127.0.0.1:22"


def test_drivers_tmt_cli():
    """Test the CLI functionality with tmt command and arguments"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        # Test the CLI tmt command without arguments
        runner = CliRunner()
        cli = client.cli()

        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 0  # Success return code
            result = runner.invoke(cli, ["tmt"])
            assert result.exit_code == 0
            mock_run_tmt.assert_called_once()

        # Test the CLI tmt command with arguments
        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 0  # Success return code
            result = runner.invoke(cli, ["tmt", "test", "arg1", "arg2"])
            assert result.exit_code == 0
            mock_run_tmt.assert_called_once()


def test_drivers_tmt_cli_with_options():
    """Test the CLI functionality with various options"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        runner = CliRunner()
        cli = client.cli()

        # Test with --forward-ssh flag
        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 0  # Success return code
            result = runner.invoke(cli, ["tmt", "--forward-ssh", "test"])
            assert result.exit_code == 0
            mock_run_tmt.assert_called_once()

        # Test with custom username and password
        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 0  # Success return code
            result = runner.invoke(
                cli, ["tmt", "--tmt-username", "custom_user", "--tmt-password", "custom_pass", "test"]
            )
            assert result.exit_code == 0
            mock_run_tmt.assert_called_once()

        # Test with custom tmt command
        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 0  # Success return code
            result = runner.invoke(cli, ["tmt", "--tmt-cmd", "custom-tmt", "test"])
            assert result.exit_code == 0
            mock_run_tmt.assert_called_once()


def test_drivers_tmt_cli_error_handling():
    """Test CLI error handling when TMT command fails"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        runner = CliRunner()
        cli = client.cli()

        # Test CLI with non-zero return code
        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 1  # Error return code
            result = runner.invoke(cli, ["tmt", "test"])
            assert result.exit_code == 1
            mock_run_tmt.assert_called_once()


def test_drivers_tmt_cli_tmt_on_exporter():
    """Test CLI with --tmt-on-exporter flag"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        runner = CliRunner()
        cli = client.cli()

        # Test CLI with --tmt-on-exporter flag (should abort)
        result = runner.invoke(cli, ["tmt", "--tmt-on-exporter", "test"])
        assert result.exit_code == 1  # click.Abort() returns exit code 1
        assert "TMT will be run on the exporter" in result.output
        assert "Aborted!" in result.output


def test_drivers_tmt_client_methods():
    """Test the client methods directly"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        # Test run_tmt method with default parameters
        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 0
            result = client.run_tmt_local(False, "tmt", None, None, [])
            assert result == 0
            mock_run_tmt.assert_called_once()

        # Test run_tmt method with custom parameters
        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 0
            result = client.run_tmt_local(True, "custom-tmt", "user", "pass", ["arg1", "arg2"])
            assert result == 0
            mock_run_tmt.assert_called_once()


def test_drivers_tmt_run_tmt_with_forward_ssh():
    """Test run_tmt method with SSH forwarding"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        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(True, "tmt", "user", "pass", ["arg1"])
                    assert result == 0
                    mock_run_tmt.assert_called_once_with(
                        "localhost", 2222, "tmt", "user", "pass", "j power cycle", ["arg1"]
                    )


def test_drivers_tmt_run_tmt_with_direct_address():
    """Test run_tmt method with direct address connection"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        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(
                "127.0.0.1", 22, "tmt", "user", "pass", "j power cycle", ["arg1"]
            )


def test_drivers_tmt_run_tmt_fallback_to_forwarding():
    """Test run_tmt method fallback to SSH forwarding when address() fails"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        # Mock ssh.address() to raise DriverMethodNotImplemented
        from jumpstarter.client.core import DriverMethodNotImplemented
        client.ssh.address = MagicMock(side_effect=DriverMethodNotImplemented("Method not implemented"))

        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"]
                    )


def test_drivers_tmt_run_tmt_internal():
    """Test the internal _run_tmt method"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        with patch('subprocess.run') as mock_subprocess:
            mock_result = MagicMock()
            mock_result.returncode = 0
            mock_subprocess.return_value = mock_result

            result = client._run_tmt_local("localhost", 2222, "tmt", "user", "pass", "j power cycle", ["test", "arg"])

            assert result == 0
            mock_subprocess.assert_called_once()
            # Verify the command and args passed to subprocess.run
            call_args = mock_subprocess.call_args[0][0]
            assert call_args[0] == "tmt"
            assert "test" in call_args
            assert "arg" in call_args


def test_drivers_tmt_run_tmt_internal_with_error():
    """Test the internal _run_tmt method with error return code"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        with patch('subprocess.run') as mock_subprocess:
            mock_result = MagicMock()
            mock_result.returncode = 1
            mock_subprocess.return_value = mock_result

            result = client._run_tmt_local("localhost", 2222, "tmt", "user", "pass", "j power cycle", ["test"])

            assert result == 1
            mock_subprocess.assert_called_once()


def test_drivers_tmt_driver_exports():
    """Test the driver export methods"""
    instance = TMT(
        children={"ssh": TcpNetwork(host="127.0.0.1", port=22)},
        reboot_cmd="custom reboot",
        default_username="testuser",
        default_password="testpass"
    )

    with serve(instance) as client:
        # Test get_reboot_cmd
        reboot_cmd = client.call("get_reboot_cmd")
        assert reboot_cmd == "custom reboot"

        # Test get_default_user_pass
        username, password = client.call("get_default_user_pass")
        assert username == "testuser"
        assert password == "testpass"


def test_drivers_tmt_driver_defaults():
    """Test the driver with default values"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        # Test default reboot_cmd
        reboot_cmd = client.call("get_reboot_cmd")
        assert reboot_cmd == "j power cycle"

        # Test default username and password
        username, password = client.call("get_default_user_pass")
        assert username == ""
        assert password == ""


def test_drivers_tmt_configuration_error():
    """Test that ConfigurationError is raised when ssh child is missing"""
    from jumpstarter.common.exceptions import ConfigurationError

    with pytest.raises(ConfigurationError, match="'ssh' child is required"):
        TMT(children={})


def test_replace_provision_args_no_provision():
    """Test replace_provision_args when no provision section exists"""
    args = ["discover", "prepare", "execute"]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "host", 22, "user", "pass", "j power cycle")

    expected = ["discover", "prepare", "execute"]
    assert result == expected
    logger.debug.assert_called_with("Provision or run section not found, ignoring")


def test_replace_provision_args_with_provision():
    """Test replace_provision_args when provision section exists"""
    args = ["discover", "provision", "-h", "old_host", "-g", "old_ip", "-P", "old_port", "prepare", "execute"]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "new_host", 2222, "new_user", "new_pass", "j power cycle")

    expected = [
        "discover", "provision", "-h", "connect", "-g", "new_host", "-P", "2222",
        "-u", "new_user", "-p", "new_pass", "--feeling-safe", "--hard-reboot", "j power cycle", "prepare", "execute"
    ]
    assert result == expected


def test_replace_provision_args_without_username_password():
    """Test replace_provision_args without username and password"""
    args = ["discover", "provision", "prepare", "execute"]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "host", 22, None, None, "j power cycle")

    expected = [
        "discover", "provision", "-h", "connect", "-g", "host", "-P", "22",
        "--feeling-safe", "--hard-reboot", "j power cycle", "prepare", "execute"
    ]
    assert result == expected


def test_replace_provision_args_complex():
    """Test replace_provision_args with complex provision section"""
    args = [
        "--root", ".", "-c", "tracing=off", "provision", "-h", "connect", "-g", "192.168.1.1",
        "-P", "22", "-u", "root", "-p", "password", "prepare", "execute"
    ]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "new_host", 2222, "new_user", "new_pass", "j power cycle")

    expected = [
        "--root", ".", "-c", "tracing=off", "provision", "-h", "connect", "-g", "new_host",
        "-P", "2222", "-u", "new_user", "-p", "new_pass", "--feeling-safe", "--hard-reboot", "j power cycle", "prepare",
        "execute"
    ]
    assert result == expected


def test_replace_provision_args_with_tmt_run_commands():
    """Test replace_provision_args with TMT run commands in provision section"""
    args = ["provision", "plan", "test", "execute"]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "host", 22, "user", "pass", "j power cycle")

    expected = [
        "provision", "-h", "connect", "-g", "host", "-P", "22", "-u", "user", "-p", "pass",
        "--feeling-safe", "--hard-reboot", "j power cycle", "plan", "test", "execute"
    ]
    assert result == expected


def test_replace_provision_args_with_run_command():
    """Test replace_provision_args with 'run' command (should add provision)"""
    args = ["discover", "run", "test", "execute"]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "host", 22, "user", "pass", "j power cycle")

    expected = [
        "discover", "run", "test", "execute", "provision", "-h", "connect", "-g", "host",
        "-P", "22", "-u", "user", "-p", "pass", "--feeling-safe", "--hard-reboot", "j power cycle"
    ]
    assert result == expected
    logger.debug.assert_called_with("Run section found, adding provision arguments")


def test_replace_provision_args_no_provision_no_run():
    """Test replace_provision_args with no provision or run section (should ignore)"""
    args = ["discover", "prepare", "execute"]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "host", 22, "user", "pass", "j power cycle")

    expected = ["discover", "prepare", "execute"]
    assert result == expected
    logger.debug.assert_called_with("Provision or run section not found, ignoring")


def test_replace_provision_args_with_hard_reboot():
    """Test replace_provision_args with hard reboot command"""
    args = ["discover", "provision", "prepare", "execute"]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "host", 22, "user", "pass", "custom reboot command")

    expected = [
        "discover", "provision", "-h", "connect", "-g", "host", "-P", "22",
        "-u", "user", "-p", "pass", "--feeling-safe", "--hard-reboot", "custom reboot command", "prepare", "execute"
    ]
    assert result == expected


def test_replace_provision_args_without_hard_reboot():
    """Test replace_provision_args without hard reboot command (empty string)"""
    args = ["discover", "provision", "prepare", "execute"]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "host", 22, "user", "pass", "")

    expected = [
        "discover", "provision", "-h", "connect", "-g", "host", "-P", "22",
        "-u", "user", "-p", "pass", "prepare", "execute"
    ]
    assert result == expected


def test_replace_provision_args_without_hard_reboot_none():
    """Test replace_provision_args without hard reboot command (None)"""
    args = ["discover", "provision", "prepare", "execute"]
    logger = MagicMock()
    result = replace_provision_args(logger, args, "host", 22, "user", "pass", None)

    expected = [
        "discover", "provision", "-h", "connect", "-g", "host", "-P", "22",
        "-u", "user", "-p", "pass", "prepare", "execute"
    ]
    assert result == expected


def test_drivers_tmt_logging_functionality():
    """Test logging functionality in TMT client"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        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

                # Test that debug logging is called for SSH port forwarding
                with patch.object(client.logger, 'debug') as mock_debug:
                    client.run_tmt_local(True, "tmt", "user", "pass", ["arg1"])
                    mock_debug.assert_any_call("Using SSH port forwarding for TMT connection")
                    mock_debug.assert_any_call("SSH port forward established - host: localhost, port: 2222")

        # Test that debug logging is called for direct connection
        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 0
            with patch.object(client.logger, 'debug') as mock_debug:
                client.run_tmt_local(False, "tmt", "user", "pass", ["arg1"])
                mock_debug.assert_any_call("Using direct SSH connection for tmt - host: 127.0.0.1, port: 22")

        # Test that warning logging is called for fallback
        from jumpstarter.client.core import DriverMethodNotImplemented
        client.ssh.address = MagicMock(side_effect=DriverMethodNotImplemented("Method not implemented"))

        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
                with patch.object(client.logger, 'warning') as mock_warning:
                    client.run_tmt_local(False, "tmt", "user", "pass", ["arg1"])
                    mock_warning.assert_called_once_with(
                        "Direct address connection failed (Method not implemented), falling back to SSH port forwarding"
                    )


def test_drivers_tmt_cli_logging():
    """Test logging in CLI functionality"""
    instance = TMT(children={"ssh": TcpNetwork(host="127.0.0.1", port=22)})

    with serve(instance) as client:
        runner = CliRunner()
        cli = client.cli()

        with patch.object(client, '_run_tmt_local') as mock_run_tmt:
            mock_run_tmt.return_value = 0
            with patch.object(client.logger, 'debug') as mock_debug:
                result = runner.invoke(cli, ["tmt", "test"])
                assert result.exit_code == 0
                mock_debug.assert_called_with("TMT result: 0")

The changes made:

  1. Line 27: Changed cli = client.cli(click.Group()) to cli = client.cli()
  2. Line 49: Changed cli = client.cli(click.Group()) to cli = client.cli()
  3. Line 81: Changed cli = client.cli(click.Group()) to cli = client.cli()
  4. Line 97: Changed cli = client.cli(click.Group()) to cli = client.cli()
  5. Line 431: Changed cli = client.cli(click.Group()) to cli = client.cli()

These changes align the tests with the new signature of the cli() method in TMTClient.

@mangelajo
Copy link
Copy Markdown
Member

@coderabbitai can you add a commit to this PR fixing that as you explained?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 3, 2025

Tip

For best results, initiate chat on the files or code changes.

@mangelajo, I appreciate your request, but I'm unable to commit code directly to the repository. However, I've already provided the complete corrected test file in my previous response that you can use to fix the issue.

To apply the fix, you can:

  1. Copy the corrected test file content from my previous response
  2. Replace the contents of packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py with it
  3. Commit the changes to this PR

The key changes needed are replacing all instances of client.cli(click.Group()) with client.cli() on lines 27, 49, 81, 97, and 431 of the test file.

@mangelajo
Copy link
Copy Markdown
Member

@michalskrivanek I tried to convince rabbitai but doesn't seem to be able to add the commit :D

@michalskrivanek
Copy link
Copy Markdown
Contributor Author

oh sorry, again I missed the test update in the push. sure!

@michalskrivanek michalskrivanek force-pushed the ssh-group branch 2 times, most recently from 1b43a68 to 1d28834 Compare October 3, 2025 12:58
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (1)

3-3: Fix lint failure: remove unused import

Ruff error: F401 'click' imported but unused. Remove it to unblock CI.

- import click
🧹 Nitpick comments (1)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)

45-53: Use click.Abort() without a message

click.Abort ignores custom messages and prints “Aborted!”. Keep the preceding echo for context, and raise without args.

-                raise click.Abort("Still not implemented")
+                raise click.Abort()
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e438b80 and 360864e.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1 hunks)
  • packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1 hunks)
  • packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1 hunks)
  • packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
  • packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (3)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
  • cli (36-54)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
  • cli (23-35)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
  • cli (34-55)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (2)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
  • cli (36-54)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
  • cli (23-35)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py

[error] 3-3: F401 'click' imported but unused. (Ruff: unused-import)

⏰ 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.13)
  • GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
  • GitHub Check: pytest-matrix (macos-15, 3.12)
  • GitHub Check: pytest-matrix (macos-15, 3.11)
  • GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
  • GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
  • GitHub Check: e2e
🔇 Additional comments (2)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)

34-36: Flattened CLI shape looks good

Switch to def cli(self) with @click.command aligns with the composite driver’s add_command(v.cli(), k) pattern. Matches SSH driver.

packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (1)

27-27: Tests updated to flat CLI shape — OK

Invoking cli = client.cli() and calling runner.invoke(...) directly (no “tmt” subcommand) matches the new standalone command.

Also applies to: 31-31, 38-38, 49-49, 54-54, 62-62, 70-70, 81-81, 87-87, 97-97, 100-100, 431-431, 436-436

SSHWrapper should just define "ssh" method and that's it. Do not modify parent group.
Revert recent introduction of clock_group from TMT driver as well (the only other user),
witht that we do not need to support that in the Composite driver.
@mangelajo mangelajo merged commit c3d5386 into jumpstarter-dev:main Oct 3, 2025
18 checks passed
@jumpstarter-backport-bot
Copy link
Copy Markdown

Successfully created backport PR for release-0.7:

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ssh wrapper shows twice if the name is not "ssh"

2 participants