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

Raspbian Backend (1st phase) #89

Merged
merged 85 commits into from
Jul 15, 2017
Merged

Conversation

ritwickdsouza
Copy link
Contributor

@ritwickdsouza ritwickdsouza commented Jul 12, 2017

Merging raspbian branch of fork to rasbian branch of original repo.

References #78.

@nemesifier nemesifier changed the title Raspbian Backend Raspbian Backend (1st phase) Jul 13, 2017
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

See my inline comments plus the following:

templates are rather unreadable. You need to improve their readability in some way, starting from the names of variables.



class RaspbianConverter(BaseConverter):
def test_function(self):
Copy link
Member

Choose a reason for hiding this comment

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

what's this for?

Raspbian Backend
"""
schema = schema
env_path = 'netjsonconfig.backends.raspbian'
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't be needed anymore, try removing it

{% if i|string() == 'general' %}
{% for general in j %}
{% if general.get('hostname') %}
config: /etc/hostname
Copy link
Member

Choose a reason for hiding this comment

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

are you sure this line doesn't make the configuration invalid when deployed on an actual device?

Shouldn't it be # config: /etc/hostname?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'll comment it out.

{% for interface in j %}
{% if interface.get('iftype') == 'wireless' %}
{% if interface.get('mode') == 'adhoc' %}
config: /etc/network/interfaces
Copy link
Member

Choose a reason for hiding this comment

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

are you sure this line doesn't make the configuration invalid when deployed on an actual device?

Shouldn't it be # config: /etc/network/interfaces?

@@ -0,0 +1,8 @@
{% for i, j in data.items() %}
{% if i|string() == 'ntp' %}
config: /etc/ntp.conf
Copy link
Member

Choose a reason for hiding this comment

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

same as previous comment

{% for i, j in data.items() %}
{% if i|string() in ['dns_servers', 'dns_search'] %}
{% if count == [1] %}
config: /etc/resolv.conf
Copy link
Member

Choose a reason for hiding this comment

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

same as previous comment

expected = ''''''
self.assertEqual(o.render(), expected)

@unittest.skip('Test skipping')
Copy link
Member

Choose a reason for hiding this comment

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

what about this test skipping thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not implemented

'''
self.assertEqual(o.render(), expected)

@unittest.skip('Test skipping')
Copy link
Member

Choose a reason for hiding this comment

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

same as previous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not implemented

'''
self.assertEqual(o.render(), expected)

@unittest.skip('Test skipping')
Copy link
Member

Choose a reason for hiding this comment

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

what about this test skipping thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not implemented as of now. So skipping them till I implement it.

nemesifier and others added 28 commits July 13, 2017 12:26
@ritwickdsouza ritwickdsouza merged commit bdf028d into openwisp:raspbian Jul 15, 2017
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.

2 participants