Skip to content

Added support to TAP mode in OpenVPN status parsing #49

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 24, 2017
Merged

Added support to TAP mode in OpenVPN status parsing #49

merged 1 commit into from
Oct 24, 2017

Conversation

rohithasrk
Copy link
Contributor

No description provided.

nemesifier
nemesifier requested changes Oct 20, 2017
View reviewed changes
tests/test_openvpn.py Outdated
@@ -19,15 +19,19 @@ def test_parse(self):
self.assertEqual(p.metric, 'static')

def test_json_dict(self):
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't look right, the two test files look different in number of links and nodes, therefore you'd better have two separate tests and not mix things up to avoid confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nemesisdesign I edited the second test file to have exactly the same number of links and nodes.

Copy link
Member

Choose a reason for hiding this comment

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

(see my comment above) I think something is not right in the code, the decrease of code coverage is a hint, it should not happen since you have not added any new line of code, so I think some lines of code that were tested before are not being tested now.
Could you try to do as suggested and ensure both tests have their own test function and are really executed?

@coveralls
Copy link

coveralls commented Oct 20, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 99.766% when pulling c3bd53f on rohithasrk:openvpn-tap into fbf8ee4 on ninuxorg:master.

nemesifier
nemesifier requested changes Oct 20, 2017
View reviewed changes
tests/static/openvpn-2-links-tap.txt Outdated
@@ -0,0 +1,12 @@
OpenVPN CLIENT LIST
Updated,Fri Oct 20 12:52:38 2017
Common Name,Real Address,Bytes Received,Bytes Sent,Connected Since
Copy link
Member

Choose a reason for hiding this comment

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

I see only 2 nodes here, am I missing something?

tests/test_openvpn.py Outdated
@@ -19,15 +19,19 @@ def test_parse(self):
self.assertEqual(p.metric, 'static')

def test_json_dict(self):
Copy link
Member

Choose a reason for hiding this comment

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

(see my comment above) I think something is not right in the code, the decrease of code coverage is a hint, it should not happen since you have not added any new line of code, so I think some lines of code that were tested before are not being tested now.
Could you try to do as suggested and ensure both tests have their own test function and are really executed?

@coveralls
Copy link

coveralls commented Oct 20, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 99.766% when pulling f3079c7 on rohithasrk:openvpn-tap into fbf8ee4 on ninuxorg:master.

@rohithasrk
Copy link
Contributor Author

@nemesisdesign There seems to be some problem with the openvpn-status. As the number of nodes it is showing is wrong. It is counting the virtual addresses as one node too, therefore doubling the number. Check Travis once.

@coveralls
Copy link

coveralls commented Oct 22, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 99.766% when pulling 79d9023 on rohithasrk:openvpn-tap into fbf8ee4 on ninuxorg:master.

@coveralls
Copy link

coveralls commented Oct 22, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling e7283ee on rohithasrk:openvpn-tap into fbf8ee4 on ninuxorg:master.

rohithasrk
rohithasrk commented Oct 22, 2017
View reviewed changes
netdiff/parsers/openvpn.py Outdated
for route in data.routing_table.values()
if route.real_address == client.real_address
if not isinstance(route.virtual_address, (
ipaddress.IPv4Network, ipaddress.IPv6Network))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we remove this check @nemesisdesign? Checking for the instances? This has been suggested by @tonyseek

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 826cfe4 on rohithasrk:openvpn-tap into fbf8ee4 on ninuxorg:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 826cfe4 on rohithasrk:openvpn-tap into fbf8ee4 on ninuxorg:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d695192 on rohithasrk:openvpn-tap into fbf8ee4 on ninuxorg:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d695192 on rohithasrk:openvpn-tap into fbf8ee4 on ninuxorg:master.

@rohithasrk
Copy link
Contributor Author

@nemesisdesign Can it be merged?

@rohithasrk rohithasrk merged commit 71ff3b7 into openwisp:master Oct 24, 2017
@rohithasrk rohithasrk deleted the openvpn-tap branch October 24, 2017 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nemesifier nemesifier nemesifier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rohithasrk @coveralls @nemesifier