Raspbian Backend #92
Raspbian Backend #92
Conversation
| @@ -0,0 +1,74 @@ | |||
| from ipaddress import IPv4Interface, ip_network | |||
There was a problem hiding this comment.
Working on it now.
| {% if i|string() == 'general' %} | ||
| {% for general in j %} | ||
| {% if general.get('timezone') %} | ||
| run commands: |
There was a problem hiding this comment.
Couldn't find a method to set timezone by modifying a config file.
So created a template which tells what command needs to be run to do the required.
There was a problem hiding this comment.
what about a script? We can then create an install script that must be launched to deploy the configuration
There was a problem hiding this comment.
I was thinking about doing the same. I will do that.
| @@ -0,0 +1,12 @@ | |||
| {% for i, j in data.items() %} | |||
There was a problem hiding this comment.
can't you just loop over data.general instead of doing this cumbersome thing? (same concept applies to other templates)
There was a problem hiding this comment.
could you also use more readable variable names than i and j please? This applies to other lines of code too.
There was a problem hiding this comment.
The templates are now much cleaner.
| from ...schema import schema as default_schema | ||
| from ...utils import merge_config | ||
|
|
||
| schema = merge_config(default_schema, {}) |
There was a problem hiding this comment.
with no schema additions we can't validate mandatory data or enforce checks, are you sure this step is not needed?
There was a problem hiding this comment.
I'll work on a schema
There was a problem hiding this comment.
@ritwickdsouza at least the minimum required stuff. This part will become clearer when we'll be able to start viewing the configuration form in web UI
| {% for wireless in data.wireless %} | ||
| # config: /etc/hostapd/hostapd.conf | ||
|
|
||
| interface={{ wireless.get('ifname') }} |
There was a problem hiding this comment.
why not {{ wireless.ifname }} instead? Jinja2 should allow you this syntax which is more concise and easy to read, sampe applies for the rest of attributes
There was a problem hiding this comment.
Yeah, I'll make the changes in all the templates.
| ieee80211ac=1 | ||
| {% endif %} | ||
| ssid={{ wireless.get('ssid') }} | ||
| {% if wireless.get('encryption') != {} %} |
There was a problem hiding this comment.
try just:
{% if wireless.get('encryption') %}
| wpa={{ wireless.get('encryption').get('wpa') }} | ||
| wpa_key_mgmt={{ wireless.get('encryption').get('wpa_key_mgmt') }} | ||
| wpa_passphrase={{ wireless.get('encryption').get('wpa_passphrase') }} | ||
| {% if wireless.get('encryption', None).get('wpa_pairwise') != 'AUTO' %} |
There was a problem hiding this comment.
The following code:
wireless.get('encryption', None).get('wpa_pairwise')
is broken because if encryption is not present, wireless['encryption'] will be None and then you will be trying to call the get method on a None object, which will raise an exception.
But if you apply my previous suggestion, this code block will be executed only if wireless.encryption is not empty or None, hence the default value in the get method is not necessary and you may write just:
{% if wireless.encryption.wpa_pairwise != 'AUTO' %}
| {{ general.get('hostname') }} | ||
|
|
||
| {% endif %} | ||
| {% endfor %} |
There was a problem hiding this comment.
can't this template be simplified a lot? Eg:
{% if data.general and general.hostname %}
# config: /etc/hostname
{{ general.hostname }}
{% endif %}| {% for interface in data.interfaces %} | ||
| {% if interface.get('iftype') in ['ethernet', 'bridge', 'loopback', 'wireless'] %} | ||
| # config: /etc/network/interfaces | ||
| {% if interface.get('address') != None %} |
There was a problem hiding this comment.
try:
{% if interface.address %}
| @@ -127,7 +127,7 @@ def render(self, files=True): | |||
| if self.intermediate_data is None: | |||
| self.to_intermediate() | |||
| # support multiple renderers | |||
| renderers = getattr(self, 'renderers', [self.renderer]) | |||
| renderers = getattr(self, 'renderers', None) or [self.renderer] | |||
There was a problem hiding this comment.
have you rebased on master yet? This change shouldn't show up because is already on master
There was a problem hiding this comment.
Ohh. I thought I created the branch after this update.
| -------------- | ||
|
|
||
| .. automethod:: netjsonconfig.Raspbian.__init__ | ||
|
|
There was a problem hiding this comment.
remove unnecessary blank line
|
|
||
| config: /etc/network/interfaces | ||
| auto eth0 | ||
| iface eth0 inet static |
There was a problem hiding this comment.
use 4 spaces indentation for consistency with other examples on the same page which are using 4
docs/source/backends/raspbian.rst
Outdated
|
|
||
| Will be rendered as follows:: | ||
|
|
||
| config: /etc/hostapd/hostapd.conf |
| Wireless AdHoc Mode | ||
| ~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| In wireless adhoc mode, the ``bssid`` property is required. |
docs/source/backends/raspbian.rst
Outdated
|
|
||
| Will be rendered as follows:: | ||
|
|
||
| config: /etc/hostapd/hostapd.conf |
docs/source/backends/raspbian.rst
Outdated
| "0.openwrt.pool.ntp.org", | ||
| "1.openwrt.pool.ntp.org", | ||
| "2.openwrt.pool.ntp.org", | ||
| "3.openwrt.pool.ntp.org" |
There was a problem hiding this comment.
use a different pool of servers otherwise it looks badly copied from the openwrt backend page :-P
docs/source/backends/raspbian.rst
Outdated
| config: /etc/network/interfaces | ||
| auto eth0.1 | ||
| iface eth0.1 inet static | ||
| address 192.168.1.1 |
There was a problem hiding this comment.
what about making indentation consistent with other examples which use 4 spaces?
docs/source/backends/raspbian.rst
Outdated
| >>> o = Raspbian({ | ||
| ... "interfaces": [ | ||
| ... { | ||
| ... "name": "eth0", |
docs/source/backends/raspbian.rst
Outdated
| After modifying the config files run the following command to change the | ||
| hostname:: | ||
|
|
||
| $ /etc/init.d/hostname.sh start |
There was a problem hiding this comment.
with the install configuration idea we can move this command there too
docs/source/backends/raspbian.rst
Outdated
|
|
||
| This will enable on the next reboot. Incase you want to activate it immediately:: | ||
|
|
||
| sudo sh -c "echo 1 > /proc/sys/net/ipv4/ip_forward" |
There was a problem hiding this comment.
can this go to the install script too?
| Now we just need to start our services:: | ||
|
|
||
| sudo service hostapd start | ||
| sudo service dnsmasq start |
docs/source/backends/raspbian.rst
Outdated
| wireless-channel 1 | ||
| wireless-essid freifunk | ||
| wireless-mode ad-hoc | ||
|
|
tests/raspbian/test_hostapd.py
Outdated
| from netjsonconfig.utils import _TabsMixin | ||
|
|
||
|
|
||
| class TestHostapdRenderer(unittest.TestCase, _TabsMixin): |
| expected = '''# config: /etc/network/interfaces | ||
|
|
||
| auto eth0 | ||
| iface eth0 inet static |
There was a problem hiding this comment.
why in the examples contained in the docs we have indentation for these? Ensure the documentation returns the same output the library generates please, otherwise users will get confused
There was a problem hiding this comment.
I had made changes to the template and not updated the docs. I'll make the changes.
1 similar comment
|
@ritwickdsouza remember the install procedure and instructions |
| @@ -169,7 +169,7 @@ method_arguments = parse_method_arguments(args.args) | |||
| backends = { | |||
| 'openwrt': netjsonconfig.OpenWrt, | |||
| 'openwisp': netjsonconfig.OpenWisp, | |||
| 'openvpn': netjsonconfig.OpenVpn | |||
References #78 and #79