scripts, tests: explicitly set confdir in the rest of server code #301

HonzaCholasta · 2016-12-02T10:53:15Z

Commit 1e6a204 added explicit confdir
setting to api.bootstrap() calls of a randomly selected portion of
server-side scripts and tests. This commit adds it to the rest of
server-side code for consistency.

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

Commit 1e6a204 added explicit confdir setting to api.bootstrap() calls of a randomly selected portion of server-side scripts and tests. This commit adds it to the rest of server-side code for consistency. https://fedorahosted.org/freeipa/ticket/6389

stlaz · 2016-12-12T08:38:27Z

install/tools/ipa-managed-entries

@@ -72,7 +73,7 @@ def main():
        sys.exit("Unrecognized action [" + args[0] + "]")
    standard_logging_setup(None, debug=options.debug)

-    api.bootstrap(context='cli', debug=options.debug)
+    api.bootstrap(context='cli', debug=options.debug, confdir=paths.ETC_IPA)


This is an ugly old script, therefore this should be in try-except block.

Nvm, the script behaves the same in other error cases, disregard the previous comment.

NACK

Same reason as for ipa-compat-manage

tiran · 2016-12-12T09:47:58Z

I'll review the patch by the end of the week. Some changes are not required.

stlaz · 2016-12-13T09:03:18Z

@tiran I find all the changes actually required. I think ACK is in order unless you spell out those which you think are not necessary.

tiran

The bootstrap calls in ipaserver/install should use confdir, most other places do not need specia cases to work.

tiran · 2016-12-15T15:04:26Z

doc/guide/wsgi.py.txt

 env._finalize_core(**dict(DEFAULT_CONFIG))

 # Initialize the API with the proper debug level
-api.bootstrap(context='server', debug=env.debug, log=None) (ref:wsgi-app-bootstrap)
+api.bootstrap(context='server', confdir=paths.ETC_IPA,
+              debug=env.debug, log=None) (ref:wsgi-app-bootstrap)


NACK

Documentation should explain the common case (no confdir override). Users may want to use the IPA_CONFDIR var here. I'm ok with a comment or second example.

This file should be kept in sync with install/share/wsgi.py. This change does just that, there is no other intention behind it.

tiran · 2016-12-15T15:06:10Z

install/tools/ipa-compat-manage

+    api.bootstrap(context='cli',
+                  in_server=True,
+                  debug=options.debug,
+                  confdir=paths.ETC_IPA)


NACK, the script works fine with IPA_CONFDIR. It does neither modify local file nor requires root permission to run.

You're still required to enter Directory Manager password which is why I would expect the script not to be influenced by an environment variable.

It does not matter at all that the script works with IPA_CONFDIR - there is no use case for it and it only introduces additional maintenance burden for us, hence the change.

If you want server scripts to work with IPA_CONFDIR, file a RFE. Until it's implemented, the correct way to handle IPA_CONFDIR in server scripts is not to support it.

I don't agree. There is no additonal maintenance burden.

Yes, there is - we will have to test this and make sure we don't break it, which is not worth doing for a "feature" nobody wants and will ever use.

tiran · 2016-12-15T15:07:20Z

install/tools/ipa-csreplica-manage

+        context='cli',
+        in_server=True,
+        verbose=options.verbose,
+        confdir=paths.ETC_IPA,


NACK

Same reason as for ipa-compat-manage

tiran · 2016-12-15T15:07:58Z

install/tools/ipa-managed-entries

@@ -72,7 +73,7 @@ def main():
        sys.exit("Unrecognized action [" + args[0] + "]")
    standard_logging_setup(None, debug=options.debug)

-    api.bootstrap(context='cli', debug=options.debug)
+    api.bootstrap(context='cli', debug=options.debug, confdir=paths.ETC_IPA)


NACK

Same reason as for ipa-compat-manage

tiran · 2016-12-15T15:09:37Z

ipaserver/advise/base.py

-        advise_api.bootstrap(in_server=False, context='cli')
+        advise_api.bootstrap(in_server=False,
+                             context='cli',
+                             confdir=paths.ETC_IPA)


NACK

Same reason as for ipa-compat-manage

tiran · 2016-12-15T15:11:07Z

ipaserver/dnssec/ldapkeydb.py

@@ -414,7 +415,8 @@ def zone_keypairs(self):
    log = ipa_log_manager.root_logger

    # IPA framework initialization
-    ipalib.api.bootstrap(in_server=True, log=None)  # no logging to file
+    # no logging to file
+    ipalib.api.bootstrap(in_server=True, log=None, confdir=paths.ETC_IPA)


NACK

Same reason as for ipa-compat-manage

This is just some debugging code in if __name__ == '__main__' block. There is no harm to support IPA_CONFDIR

tiran · 2016-12-15T15:12:56Z

ipatests/test_ipaserver/test_ldap.py

@@ -111,7 +112,7 @@ def test_Backend(self):
        # a client-only api. Then we register in the commands and objects
        # we need for the test.
        myapi = create_api(mode=None)
-        myapi.bootstrap(context='cli', in_server=True)
+        myapi.bootstrap(context='cli', in_server=True, confdir=paths.ETC_IPA)


NACK

Tests use their own confdir for testing. Let's handle the env var in the test runner.

This API object does not use the test confdir. The test runner should be updated in different PR.

tiran · 2016-12-15T15:13:08Z

ipatests/test_ipaserver/test_serverroles.py

-    test_api.bootstrap(in_server=True, ldap_uri=api.env.ldap_uri)
+    test_api.bootstrap(in_server=True,
+                       ldap_uri=api.env.ldap_uri,
+                       confdir=paths.ETC_IPA)


NACK

same reason as above

stlaz · 2017-02-22T07:58:26Z

This PR implements the stuff that was agreed on in later comments in #280 and actually requested by @pvoborni.
Currently, I do not see the reason why this PR should not be accepted, if IPA_CONFDIR is required in either of these scripts for some reason, we can implement it later and add the justification to it.

HonzaCholasta · 2017-02-22T08:01:56Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/fe6f2b6f6effcf9f3c58e1e3f6d0874609c10c25

tiran · 2017-02-22T08:02:14Z

My philosophy is: Don't fix it, if it ain't broken.

tiran self-assigned this Dec 2, 2016

stlaz reviewed Dec 12, 2016

View reviewed changes

tiran requested changes Dec 15, 2016

View reviewed changes

stlaz added the ack Pull Request approved, can be merged label Feb 22, 2017

HonzaCholasta added the pushed Pull Request has already been pushed label Feb 22, 2017

HonzaCholasta closed this Feb 22, 2017

scripts, tests: explicitly set confdir in the rest of server code #301

scripts, tests: explicitly set confdir in the rest of server code #301

HonzaCholasta commented Dec 2, 2016 •

edited

stlaz Dec 12, 2016

stlaz Dec 13, 2016

tiran Dec 15, 2016

tiran commented Dec 12, 2016

stlaz commented Dec 13, 2016

tiran left a comment

tiran Dec 15, 2016

HonzaCholasta Dec 19, 2016

tiran Dec 15, 2016

stlaz Dec 16, 2016

HonzaCholasta Dec 19, 2016 •

edited

tiran Jan 4, 2017

HonzaCholasta Feb 20, 2017

stlaz Feb 20, 2017

tiran Dec 15, 2016

tiran Dec 15, 2016

tiran Dec 15, 2016

tiran Dec 15, 2016

tiran Dec 15, 2016

HonzaCholasta Dec 19, 2016

tiran Dec 15, 2016

stlaz commented Feb 22, 2017

HonzaCholasta commented Feb 22, 2017

tiran commented Feb 22, 2017 •

edited

scripts, tests: explicitly set confdir in the rest of server code #301

scripts, tests: explicitly set confdir in the rest of server code #301

Conversation

HonzaCholasta commented Dec 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Dec 12, 2016

stlaz commented Dec 13, 2016

tiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta Dec 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Feb 22, 2017

HonzaCholasta commented Feb 22, 2017

tiran commented Feb 22, 2017 • edited

HonzaCholasta commented Dec 2, 2016 •

edited

HonzaCholasta Dec 19, 2016 •

edited

tiran commented Feb 22, 2017 •

edited