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

Python depends for telnet tls #1638

Merged
merged 1 commit into from Jul 28, 2018
Merged

Python depends for telnet tls #1638

merged 1 commit into from Jul 28, 2018

Conversation

robotfactory
Copy link
Contributor

Brief overview of PR changes/additions

Allows for tls on telent out of the box when starting up a docker container

Addresses feature request in issue #1637

Installs py2-openssl as well as cryptography, pyasn1, and service_identity

Motivation for adding to Evennia

Allows for tls on telent out of the box when starting up a docker container

Other info (issues closed, discussion etc)

Addresses feature request in issue evennia#1637

Installs py2-openssl as well as cryptography, pyasn1, and service_identity
@inquisitivecrystal
Copy link
Contributor

I'm just a random passerby, but... What's the motivation for adding the PIP dependencies to the Dockerfile, rather than requirements.txt? Are they only applicable to Docker containers?

@robotfactory
Copy link
Contributor Author

On my first look it appeared to only be an issue when launching in Docker. The more I thought about it, the more it made sense to put it in requirements.txt. I don't know if maintainers had a reason for NOT listing in requirements.txt, though. So that's worth discussion.

@Griatch
Copy link
Member

Griatch commented Jul 28, 2018

The reason for not having the pycrypto dependency in the requirement file is because not everyone wants or care about Telnet encryption (it's not active by default) so this is an optional install. Also, using the crypto required/requires some additional setup that was not something we wanted to incur on new users.

The dockerfile is a different thing in that respect - there it becomes a hindrance to not have the packages available.

We'll need to rework the dockerfiles for the develop branch of Evennia anyway, so this has not been a priority. But it sounds reasonable to me to merge this for master for now.

@Griatch Griatch merged commit 4d2ce10 into evennia:master Jul 28, 2018
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.

None yet

3 participants