[Freeipa-devel] [freeipa PR#492][opened] [WIP] config: remove meaningless defaults

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/492
Author: HonzaCholasta
Title: #492: [WIP] config: remove meaningless defaults
Action: opened

PR body:
"""
**ipalib.constants: Remove default domain, realm, basedn, xmlrpc_uri, ldap_uri**

Domain, realm, basedn, xmlrpc_uri, ldap_uri do not have any reasonable default.
This patch removes hardcoded default so the so the code which depends
on these values blows up early and does not do crazy stuff
with default values instead of real ones.

This should help to uncover issues caused by improper ipalib
initialization.

**config: provide defaults for `xmlrpc_uri`, `ldap_uri` and `basedn`**

Derive the default value of `xmlrpc_uri` and `ldap_uri` from `server`.
Derive the default value of `basedn` from `domain`.

This supersedes @pspacek's PR #113.
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/492/head:pr492
git checkout pr492

[tiran]

URL: https://github.com/freeipa/freeipa/pull/492
Title: #492: [WIP] config: remove meaningless defaults

tiran commented:
"""
https://github.com/HonzaCholasta/freeipa/blob/4ebf4b907213c9951eb9cbd276e0460552563fb1/ipalib/config.py#L579 initializes server from jsonrpc_uri. Does it make sense move this block before your new code?
"""

See the full comment at https://github.com/freeipa/freeipa/pull/492#issuecomment-281369964

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/492
Title: #492: [WIP] config: remove meaningless defaults

HonzaCholasta commented:
"""
@tiran, not really, the order does not matter here.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/492#issuecomment-281373944

[tiran]

URL: https://github.com/freeipa/freeipa/pull/492
Title: #492: [WIP] config: remove meaningless defaults

tiran commented:
"""
It does matter. In the current version ```if 'server' not in self:``` is checked and ```self.server``` is checked a couple of lines after ```if 'ldap_uri' not in self and 'server' in self:```.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/492#issuecomment-281379332

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/492
Title: #492: [WIP] config: remove meaningless defaults

HonzaCholasta commented:
"""
I stand corrected, but it does not make sense to reorder the code as you suggested anyway, as it would change the current default of `server` when only `xmlrpc_uri` is specified in the configuration from "use the hostname from `xmlrpc_uri`" do "do not set a default value".
"""

See the full comment at https://github.com/freeipa/freeipa/pull/492#issuecomment-281406314

[tiran]

URL: https://github.com/freeipa/freeipa/pull/492
Title: #492: [WIP] config: remove meaningless defaults

tiran commented:
"""
Can you add a comment to explain the order of checks and assignments? Without explanation, it's going to confuse the next poor developer.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/492#issuecomment-281597346

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/492
Title: #492: [WIP] config: remove meaningless defaults

HonzaCholasta commented:
"""
Sure.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/492#issuecomment-281597461

[tiran]

URL: https://github.com/freeipa/freeipa/pull/492
Title: #492: [WIP] config: remove meaningless defaults

tiran commented:
"""
It's probably easier to always define options like ```'ldap_uri``` but use ```None``` as default.

```
cd .; ./makeaci --validate
./makeaci: ipaserver/plugins/dogtag.py:244: ignoring ImportError: No module named backports_abc
./makeaci: ipaserver/plugins/dogtag.py:244: ignoring ImportError: No module named rnc2rng
Traceback (most recent call last):
File "./makeaci", line 134, in <module>
main(options)
File "./makeaci", line 107, in main
api.finalize()
File "/freeipa/ipalib/plugable.py", line 747, in finalize
self._get(plugin)
File "/freeipa/ipalib/plugable.py", line 776, in _get
instance = self.__instances[plugin] = plugin(self)
File "/freeipa/ipaserver/plugins/ldap2.py", line 72, in __init__
ldap_uri = api.env.ldap_uri
AttributeError: 'Env' object has no attribute 'ldap_uri'
Exception AttributeError: "'ldap2' object has no attribute
'id'" in <bound method ldap2.__del__ of ipaserver.plugins.ldap2.ldap2()> ignored
make: *** [acilint] Error 1
Makefile:1108: recipe for target 'acilint' failed
```
"""

See the full comment at https://github.com/freeipa/freeipa/pull/492#issuecomment-281760358

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/492
Author: HonzaCholasta
Title: #492: [WIP] config: remove meaningless defaults
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/492/head:pr492
git checkout pr492

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/492
Author: HonzaCholasta
Title: #492: [WIP] config: remove meaningless defaults
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/492/head:pr492
git checkout pr492

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/492
Author: HonzaCholasta
Title: #492: [WIP] config: remove meaningless defaults
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/492/head:pr492
git checkout pr492

[HonzaCholasta]

URL: https://github.com/freeipa/freeipa/pull/492
Title: #492: [WIP] config: remove meaningless defaults

HonzaCholasta commented:
"""
I took the hard way and removed the URI argument from `ldap2.__init__()`.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/492#issuecomment-285003106