Adcli: adding class and methods for adcli #145
Conversation
shridhargadekar
force-pushed
the
adcli_0
branch
from
d6f3db9 to
1052062
Compare
sssd_test_framework/utils/adcli.py
Outdated
| def _join( | ||
| self, | ||
| *, | ||
| domain: str| None = None, |
There was a problem hiding this comment.
There should be probably either domain or domain_controller.
The manual page states that it expects domain so I would go with that.
Getting both None or both populated is a recipe for a disaster.
There was a problem hiding this comment.
I'd like to bring in Sumit as well.
I don't really know if we have use for this in SSSD, I don't really think so. I'm fine with including the patch though. However, in this case, operation that changes something must be also able to revert that change. In this case join, so this should be MultihostReentrantUtility.
sssd_test_framework/utils/adcli.py
Outdated
| self.fs: LinuxFileSystem = fs | ||
| """Filesystem utils.""" | ||
|
|
||
| def _info( |
There was a problem hiding this comment.
Why do you use underscores on the methods name? Those should be all public methods, right?
sssd_test_framework/utils/adcli.py
Outdated
| self, | ||
| *, | ||
| domain: str | None = None, | ||
| domain_controller: str | None, |
sssd_test_framework/utils/adcli.py
Outdated
| def _join( | ||
| self, | ||
| *, | ||
| domain: str| None = None, | ||
| domain_controller: str| None = None, | ||
| domain_realm: str| None = None, | ||
| host_fqdn: str| None = None, | ||
| host_keytab: str| None = None, | ||
| computer_name: str| None = None, | ||
| login_ccache: str| None = None, | ||
| login_user: str| None = None, | ||
| login_type: str| None = None, | ||
| domain_ou: str| None = None, | ||
| service_name: str| None = None, | ||
| os_name: str| None = None, | ||
| os_version: str| None = None, | ||
| os_service_pack: str| None = None, | ||
| user_principal: str| None = None, | ||
| trusted_for_delegation: str| None = None, | ||
| dont_expire_password: str| None = None, | ||
| add_service_principal: str| None = None, | ||
| description: str| None = None, | ||
| setattrs: str| None = None, | ||
| no_password: str| None = None, | ||
| promp_password: str| None = None, | ||
| stdin_password: str| None = None, | ||
| one_time_password: str| None = None, | ||
| show_password: str| None = None, | ||
| show_details: str| None = None, | ||
| add_samba_data: str| None = None, | ||
| samba_data_tool: str| None = None, | ||
| ldap_passwd: str| None = None, |
There was a problem hiding this comment.
Use **kwargs or *args to handle additional argument and keep only required arguments here. Otherwise it would be difficult to maintain.
Hi, on which level the revert should work. E.g. before calling bye, |
Adding adcli class, and methods including info, discovery, join
shridhargadekar
force-pushed
the
adcli_0
branch
from
1052062 to
dfdf2a7
Compare
It's a question of how is it going to be used. Right now, in sssd-test-framework, we have this: https://github.com/SSSD/sssd-test-framework/blob/master/sssd_test_framework/topology_controllers.py#L74
In adcli, I assume that Shridar is going to call adcli.join() insinde a test. So the logic should be: MultihostUtility (cannot be used in MultihostHost objects, only in MultihostRole objects inside tests):
MultihostReentrantUtility (cannot be used in MultihostHost objects and in MultihostRole objects):
Docs: https://pytest-mh.readthedocs.io/en/latest/articles/extending/multihost-utilities.html Other way would be to rely on the backup/restore mechanisms to make sure everything is reverted after each test. But in this case, it needs to be thoroughly documented so the expectation is clear. That might be enough. So it really depends on how is it going to be used. |
|
@pbrezina, @shridhargadekar : We will probably need a way to either skip topology setup/teardown (no enrolling) so we can join ourselves or create topologies like KnownTopology.UnjoinedAD that will have the machines but not enrolled client. We can use these for example for tests like ldap sasl authid that needs a specific setup (--user-principal=host/name@REALM) to work correctly. |
Adding adcli class, and methods including info, discovery, join