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

Jump to bottom

config: remove meaningless defaults #492

Closed
wants to merge 6 commits into from
Closed

config: remove meaningless defaults #492

wants to merge 6 commits into from

Conversation

HonzaCholasta
Copy link
Contributor

@HonzaCholasta HonzaCholasta commented Feb 21, 2017 •
edited by MartinBasti

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.

@HonzaCholasta HonzaCholasta mentioned this pull request Feb 21, 2017
Closed
@tiran
Copy link
Member

tiran commented Feb 21, 2017

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?

@HonzaCholasta
Copy link
Contributor Author

@tiran, not really, the order does not matter here.

@tiran
Copy link
Member

tiran commented Feb 21, 2017

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:.

@HonzaCholasta
Copy link
Contributor Author

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".

@tiran
Copy link
Member

tiran commented Feb 22, 2017

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

@HonzaCholasta
Copy link
Contributor Author

Sure.

@tiran
Copy link
Member

tiran commented Feb 22, 2017

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

tiran added a commit to tiran/freeipa that referenced this pull request Feb 27, 2017
@tiran
Re-add ipapython.config.config for backwards compatibilty
2ad5c38 
IPAConfig, config and init_config were removed in rev 7b966e8. Ipsilon uses
ipapython.config to get realm, domain and server of an enrolled host. Re-add
a simplified version that reads settings from api.env. init_config() does
not perform DNS discovery.

Depends on PR freeipa#492 to get meaningful defaults.

https://fedorahosted.org/freeipa/ticket/6707

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran tiran mentioned this pull request Feb 27, 2017
Closed
@HonzaCholasta
Copy link
Contributor Author

I took the hard way and removed the URI argument from ldap2.__init__().

@MartinBasti MartinBasti mentioned this pull request Jun 22, 2017
Closed
@martbab martbab self-assigned this Jun 28, 2017
@martbab martbab added the ack Pull Request approved, can be merged label Jun 28, 2017
@martbab
Copy link
Contributor

martbab commented Jun 28, 2017

Regardless of what branch status widget shows the PR seems to be in need of rebase.

@MartinBasti MartinBasti changed the title [WIP] config: remove meaningless defaults config: remove meaningless defaults Jun 28, 2017
Jan Cholasta and others added 6 commits June 29, 2017 11:03
user, migration: use LDAPClient for ad-hoc LDAP connections
87826e9 
Use LDAPClient instead of ldap2 for ad-hoc remote LDAP connections in the
user_status and migrate-ds plugins.
{ca,kra}instance: drop redundant URI argument from ad-hoc ldap2 conne…
ecda2c9 
…ctions

Use the default LDAP URI from api.env.ldap_uri instead of specifying a
custom URI in the argument, as the custom URI is always the same as the
default URI.
test_ldap: drop redundant URI argument
e9d2a76 
Use the default LDAP URI from api.env.ldap_uri instead of specifying a
custom URI in the argument. The default URI might be ldapi://, so make sure
autobind is not attempted where the custom URI was ldap://.
ldap2: remove URI argument from ldap2 constructor
9bdc257 
LDAPClient should be used for ad-hoc connections, so the argument is not
necessary, and currently also unused.
@pspacek
ipalib.constants: Remove default domain, realm, basedn, xmlrpc_uri, l…
1a2194a 
…dap_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
fa39b5e 
Derive the default value of `xmlrpc_uri` and `ldap_uri` from `server`.
Derive the default value of `basedn` from `domain`.
@MartinBasti
Copy link
Contributor

master:

  • e9cb74f user, migration: use LDAPClient for ad-hoc LDAP connections
  • 935fcae {ca,kra}instance: drop redundant URI argument from ad-hoc ldap2 connections
  • 8f849a7 test_ldap: drop redundant URI argument
  • 4736fef ldap2: remove URI argument from ldap2 constructor
  • 3f6411a ipalib.constants: Remove default domain, realm, basedn, xmlrpc_uri, ldap_uri
  • ba3963b config: provide defaults for xmlrpc_uri, ldap_uri and basedn

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Jul 4, 2017
@MartinBasti MartinBasti closed this Jul 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@martbab martbab

Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
5 participants
@HonzaCholasta @tiran @martbab @MartinBasti @pspacek