Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

conn: add container/podman/docker connection #67

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

pbrezina
Copy link
Contributor

This pull requests introduce a generic connection class so we
are no longer bounded to SSH. It also implements a container
connection so podman or docker can be used instead.

This brings some breaking changes:

  • mhc.yaml: ssh configuration was replaced with conn
  • host.ssh was replaced with host.conn
  • generic classes should be used instead of SSH specific: SSHClient -> Connection, SSHProcess -> Process, SSHProcessResult -> ProcessResult and so on.

Example configuration for SSH:

  - hostname: dc.ad.test
    role: ad
    os:
      family: windows
    conn:
      type: ssh
      username: [email protected]
      password: vagrant
    config:
      adminpw: vagrant
      client:
        ad_domain: ad.test

Example configuration for podman:

  - hostname: master.ipa.test
    role: ipa
    conn:
      type: podman
      name: ipa
      sudo: True
    config:
      client:
        ipa_domain: ipa.test
        krb5_keytab: /enrollment/ipa.test.keytab
        ldap_krb5_keytab: /enrollment/ipa.test.keytab

Previously, we would try to connect also to hosts that are not required
by selected test. Those hosts are likely to be offline, therefore
--mh-lazy-ssh was wrongly required.
We will add a generic connection API so other methods besides ssh
can be used. This call is used to login to the host as given user
and to test the ssh connection. It is test specific and if it is
needed it should be provided by the test framework built on
pytest-mh, not by pytest-mh itself.
@pbrezina
Copy link
Contributor Author

Documentation is not included. After this is merged, I plan to restructuralize and rewrite the documentation so it can contain all the new features we added lately.

This replaces current SSH connection class with generic interface.
SSHClient now implements this interface. This allows us to write
additional connectors like podman or winrm.

This is a big change that breaks backwards compatibility:

 * `MultihostHost.ssh: SSHClient`  was replaced by `MultihostHost.conn:Connection`
@jakub-vavra-cz
Copy link
Collaborator

Why not keep the "ssh:" section intact?
It would be better to add an additional configuration item "conn_type:" instead with default value ssh. That can be switched to podman or docker.

@pbrezina
Copy link
Contributor Author

Why not keep the "ssh:" section intact? It would be better to add an additional configuration item "conn_type:" instead with default value ssh. That can be switched to podman or docker.

I want to keep the amount of reserved keywords minimal. This way, we have reserved only "conn", instead of "conn_type", "ssh", "podman" and "docker".

@justin-stephenson
Copy link
Contributor

Example configuration for podman:

  - hostname: master.ipa.test
    role: ipa
    conn:
      type: podman
      name: ipa
   ...

Why not container instead of name here? When first looking at this it wasn't clear to me what name should be (arbitrary or otherwise)

@pbrezina
Copy link
Contributor Author

Example configuration for podman:

  - hostname: master.ipa.test
    role: ipa
    conn:
      type: podman
      name: ipa
   ...

Why not container instead of name here? When first looking at this it wasn't clear to me what name should be (arbitrary or otherwise)

Great idea. Thank you. Done.

Copy link
Contributor

@justin-stephenson justin-stephenson left a comment

Choose a reason for hiding this comment

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

Ack, thank you.

Copy link

@danlavu danlavu left a comment

Choose a reason for hiding this comment

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

The code looks good! I haven't had a chance to test it yet. Do you want me to approve it, or can I get to it early next week?

Copy link
Collaborator

@thalman thalman left a comment

Choose a reason for hiding this comment

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

Great work, thanks

@pbrezina pbrezina merged commit 3efb30f into next-actions:master Aug 2, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants