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

Dockerize LdapCherry #26

Closed
wants to merge 6 commits into from
Closed

Conversation

smacz42
Copy link
Contributor

@smacz42 smacz42 commented Mar 28, 2019

The environment is set up in the Dockerfile. The /etc/ldapcherry/ldapcherry.ini file is able to be either used regularly (which is exposed as a volume) or configured with environment variables that are passed to the container at runtime. This is done using the init.py script, which kicks off the main ldapcherryd process after setting up the conf file.

This is all done with python2, with an eye to migrating it to python3 when the application changes have been made.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 80.757% when pulling 43c1182 on smacz42:andrewcz-homelab-179 into 3b58f14 on kakwa:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 80.757% when pulling 43c1182 on smacz42:andrewcz-homelab-179 into 3b58f14 on kakwa:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 80.757% when pulling 43c1182 on smacz42:andrewcz-homelab-179 into 3b58f14 on kakwa:master.

@coveralls
Copy link

coveralls commented Mar 28, 2019

Coverage Status

Coverage increased (+0.9%) to 80.757% when pulling 7317ebd on smacz42:andrewcz-homelab-179 into 3b58f14 on kakwa:master.

@kakwa
Copy link
Owner

kakwa commented Mar 28, 2019

Thank you for the PR, but unfortunately, I'm going to reject this PR for various reasons:

  • using debug in production is not recommended because it is unsafe as mentioned in the documentation: https://ldapcherry.readthedocs.io/en/latest/deploy.html?highlight=debug#logging
  • I'm not a big fan of hard pinning version like you are submitting with CherryPy==17.3.0. The reason being that I prefer to provoke a clean break instead having my users install obsolete and potentially insecure dependencies. I tried to chose the dependencies of ldapcherry carefully, with projects being cautious about breaking APIs (cherrypy has been quite good at it until now). I also try to target stable versions of mainstream distributions (Debian and CentOS/RHEL).
  • the init.py file feels hacky, specially "line by line" reading of the config file (instead of using an ini parser such as https://docs.python.org/3/library/configparser.html), It also doesn't account for the more complex configuration structure like the attributes and roles files, which cannot be be mapped using environment variable (equivalent to key/value config).

Lastly, I'm not comfortable in having to maintain these additional items (keep in mind ldapcherry is a personal project I maintain on my spare time).

I'm sorry to reject it, but keep in mind every PR is welcomed. Even if not integrated, it can lead to changes that are interesting, for example if you have suggestion around making ldapcherry more "Docker compatible", it is welcomed.

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.

4 participants