-
Notifications
You must be signed in to change notification settings - Fork 115
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
Drop external dependencies and reduce usage of root #225
Conversation
Thanks mate. |
@jotyGill I'll have a look at this, this week, also to ensure it works on routerOS... |
@jotyGill Hi, Finally got around to reviewing and testing. The code was suddenly highly unstable, most likely due to no testing, so it's a good thing we review each other's code when we get the chance @ranisalt Nice work, I've learned a couple of neat things from reviewing the code, just don't forget to run the code and run some tests before opening a pull request @ranisalt I can help you with documentation on how to setup Atom for usage with Python to enable linting and other features if you're interested Regarding changes I had to make, to make all working again :), see commit: 789717b I've opened a pull request containing this one, #226 This also includes a bugfix, 5b10756 |
I usually avoid external tools, e.g. I only have installed unzip and wget because of this (I use gzip and curl)
I don't use Atom, but it could be useful anyway to have a style guide. Oh, I'm thinking on creating a support library to deal with systemd, instead of calling subprocess when needed. |
I've been using this patched version for a few days now without any issues, no loss of connection, no errors, nothing out of usual, what exactly do you point as "highly unstable"? |
@ranisalt Hi, By "highly unstable" I meant the code would break at various points depending on the arguments passed to openpyn, but no worries it has been fixed in the new pull request... |
I am trying this branch and while everything installs fine I then get this on connection:
I have installed with On
Sorry to paste this here but I figured that @ranisalt would be interested. |
I have removed the need for superuser access to store logs and VPN configuration, relying on user home locations instead. That makes it only needed to use root for connection, and I think that can be avoided too, so openpyn can be installed per user.
I also removed the need for external software such as
unzip
andwget
by replacing with Python dependencies (zipfile
andrequests
, respectively).Edit: I intend to remove other
popen
calls if possible, replacing with Python code instead.