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

Add docker container build of application to repo #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smacz42
Copy link
Contributor

@smacz42 smacz42 commented Aug 19, 2019

Your suggestions in #26 (comment) 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 #36, #35, #29, and #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
Copy link

Coverage Status

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

if val.isdigit():
return val
if val in ['True', 'False']:
return val
Copy link

@Jyrno42 Jyrno42 Dec 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be val == 'True'?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Atleast for yaml files True will result in string True inside the file instead of boolean True.

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