bitinerant / bitburrow Goto Github PK

The remaining line length PEP8 issues in main.py are within a complex shell script. This is the last remaining style issue that I'm not comfortable fixing as @bitinerant knows more about how this should work.

Once that is fixed, I think we're ready to start using black to automatically format all of the projects code and then it's mechanical how things should look and everything will pass style checks all the time.

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L705

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L870

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L940-L943

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L1068
https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L1072

See function wifi_available_ssids()

See:

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L468

and elsewhere where paramiko may block.

I was able to fix all but the following three style issues:

./main.py:893:5: E722 do not use bare 'except'
./main.py:900:5: E722 do not use bare 'except'
./main.py:928:5: E722 do not use bare 'except'

The proper thing to do in these situations is to catch an exact (or several distinct) exceptions you are expecting to handle.

For example to catch two types of exceptions:

except (IDontLikeYouException, YouAreBeingMeanException) as e:
    pass

This is important as bare excepts swallow up unexpected exceptions and make debugging bugs harder.

When this is done I can complete #36

Using mypy makes the code more readable, easier to refactor, and frequently catches bugs (source).

This is needed after first router reboot.

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L147-L149

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L1067

Add a unit test for this too.

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L989

Add logging capabilities to assist in local debugging, remote (end-user) debugging, and especially when implementing new hardware or VPN providers. Be sure passwords do not go into logs. See also podcast on Eliot, especially question near the end, "How does Eliot compare ...".

Only using 'mssfix' when needed should provide better throughput on correct links. See setting up mtu, fragment, mssfix. Even better would be to determine the actual 'best' MTU setting for the particular connection.

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L327-L331

@bitinerant As mentioned in #5, if we can remove our dependency on NetworkManager/DBus we can make this project much more portable.

I'd recommend that all of dependencies by installable by PyPI and ideally have support for Windows and macOS as that will be what a majority of potential users will have. It looks like all of our current dependencies satisfy this except for NetworkManager / DBus.

One possibility is pywifi.

Handle duplicate SSIDs in conf file that have different passwords, and try factory password even if SSID already found (in case router was reset).

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L855-L858
https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L860-L864

Currently it's allowed to fail until #35, #32, and a few more minor fixes are applied.
We can then moving forward have a way to always keep things passing PEP8 properly.

Trigger

"a robust network automation toolkit written in Python that was designed for interfacing with network devices"

• OpenWrt not yet supported: issue #85

NetJSONConf

"a python library that converts NetJSON DeviceConfiguration objects into real router configurations that can be installed on systems like OpenWRT, LEDE or OpenWisp Firmware"

• may be useful in some contexts - can convert a 'configuration dictionary' to and from the format used natively by a router
• see basic concepts

NAPALM

"Network Automation and Programmability Abstraction Layer with Multivendor support"

• OpenWrt not supported: supported devices

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L946-L947

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L990-L991

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L919

Currently the instructions assume a Linux or Mac host:

Telnet to the router: telnet 192.168.8.1

I think it would be helpful to tell the user to "Open a Terminal" and input those commands.

We could also expand the the instructions for Windows 10 users by having them install the Windows Subsystem for Linux. It's as simple as installing it from the Windows Store now.

Edit: Though if the purpose is just to document the steps to later automate, this might not be necessary.

Replace this code:

os.system('nmcli dev wifi rescan 2>/dev/null')

    parser.add_argument(
        "command",
        choices=("set-up", "update", "shell", "internal-tests"),
        metavar="command",
        help=_("task to perform: set-up, update, or shell"),
    )

This would better be expressed using an Enum, which is type safe and supports better type hinting and auto-completion than bare string names.

See PyYAML dump format.

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L521-L525

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L944-L945

Currently there is a comment at the top of main.py that " " should be localized and ' ' should not be. A true solution to this would be to support full localization. One such package that does this well in Django: https://docs.djangoproject.com/en/stable/topics/i18n/translation/

We hopefully wouldn't need everything Django provides (web server, etc) and could just pull out the translation portions.

Translation would then look something like:

from django.utils.translation import gettext as _

def my_view(request):
    output = _("Welcome to my site.")

One possibility is to define from and to version numbers for set of commands, i.e. rather than
--- group dns1 --- use --- group dns 0->1 ---. In other words, this set of commands upgrades the dns group version from 0 to 1.

Note:
https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L1011-L1012
and:
https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L1025-L1026
and:
https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L1034
and:
https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L1076

This will allow hosting versions of PyPI.

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L929

main.py shell
...
Unable to find WiFi network for a supported router

When running on a VM that is NAT'd behind a host that is already connected to the router's WIFI, it would be nice if this step could be skipped automatically (perhaps with a warning logged to the terminal).

In this case, my VM can ping the router (through the NAT) so the remaining steps should work.

https://github.com/bitinerant/cleargopher/blob/d5a9a15a6f40d4e14d84bd1e5b8676d2f7f69f2f/main.py#L172-L174