kakwa / ldapcherry

Web UI for managing users and groups in multiple directory services.
MIT License
225 stars 70 forks source link

Add docker container build of application to repo #51

Open smacz42 opened 5 years ago

smacz42 commented 5 years ago

Your suggestions in https://github.com/kakwa/ldapcherry/pull/26#issuecomment-477804508 encouraged me to take a couple of months to take what you discussed with me and rewrite what LdapCherry in a docker container might look like.


First of all, LdapCherry works excellently as-is in a docker container! The demo is very impressive and easy to try out when it's distributed in a container:

$ docker run -p 8080:8080 smacz/ldapcherry:andrewcz-homelab-179

Using it this way avoids many of the mistakes that can be made by new users when installing it manually (see https://github.com/kakwa/ldapcherry/issues/36, https://github.com/kakwa/ldapcherry/issues/35, https://github.com/kakwa/ldapcherry/issues/29, and https://github.com/kakwa/ldapcherry/issues/28). It can also help them to get a feel of what they can do with your project.


In the last PR, there were definitely a number of valid concerns that you pointed out. Those tips that you gave me worked very well in figuring out how to re-write this so that it was as robust as possible.

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

This was even pointed out in the docs, and a bit of a dumb mistake on my part. So what this image now offers is an environment variable that can be passed at runtime (DEBUG) that allows the option of running in debug mode for testing purposes, or running normally (which is the default).

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

Several issues have been filed with this exact same issue! If we follow your advice though, it works perfectly. For example, this image is based on Debian stretch and as such uses the requirements-stretch.txt requirements file.

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

There was definitely a feeling of 'hackiness' to the script, and I've attempted to clean it up by logically separating functions and commenting liberally throughout.

The init.py file now reads the config file using configparser, and formats it accordingly. It also accounts for complex configuration structures of the attributes and roles file by using python's yaml library. It also handles the attributes and roles files as it should.

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 recognize that this is not a sponsored project and something that you are releasing and maintaining out of your own goodwill, and I am very thankful that you are doing this. I've made the script adaptable to most if not all changes that could happen in the forseeable future, including adapting to any backend or multiple backends, and handling additional configuration options or changes to existing ones.

The one thing that I did not do was to move all of the docker-related files into a separate directory to clean up the appearance of the repository, however that would be mainly an aesthetic change that I could make based on your own personal preference.

I also wanted to make sure that the documentation was up-to-date, so I updated the README with additional info. Please let me know where you would prefer that to be put -- if you would prefer it to be in the generated documentation rather than in the README.


While this was similarly a labor of love for me as maintaining this project is for you, if this doesn't help you or this project, there is no reason why you should include this into the main repo. And if this makes it easier for you to test new features or provide an easier way to reproduce bugs, it's only because this is a well-written program in the first place that allowed it to be so flexible as to fit in with this new technology.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 80.757% when pulling dc44455d17c6823f734aa6643fa56a4f785f6a3f on smacz42:add_docker into 856157af799dc54ab66ef0e58d0c68fdcfb9b5cc on kakwa:master.