-
Notifications
You must be signed in to change notification settings - Fork 17
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
[misc] Python3 support, Github CI and QA Checks #59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it will work after rebasing on current master.
3ac707a
to
c8a4566
Compare
This comment has been minimized.
This comment has been minimized.
7284efd
to
75f77fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my initial review. I will look up the code again to see if anything got left out.
@purhan it will be easier for you later to squash commits later if you keep commits on this PR atomic. Remember we will merge 4 commits in the end.
We should look for a workaround for now since rest of OpenWISP currently supports Python 3.6 and 3.7. diff --git a/tests/test_snmp/test_airos.py b/tests/test_snmp/test_airos.py
index 56234d0..fc190e2 100644
--- a/tests/test_snmp/test_airos.py
+++ b/tests/test_snmp/test_airos.py
@@ -168,4 +168,7 @@ class TestSNMPAirOS(unittest.TestCase, MockOutputMixin):
self.assertIsInstance(self.device.uptime_tuple, tuple)
def tearDown(self):
- self.getcmd_patcher.stop()
+ try:
+ self.getcmd_patcher.stop()
+ except RuntimeError:
+ return Don't forget to add a comment stating why this has been done and to remove it once support for Python 3.8 an above is added. |
u'Alessandro Bucciarelli, Federico Capoano', | ||
'netengine', | ||
'One line description of project.', | ||
'Miscellaneous', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we update this since we are here or shall we do this later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may be better to do this in #56
Or we can do this here, what would you suggest changing this to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for #56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We have to configure the coveralls bot to comment on PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made sure coveralls is enabled: https://coveralls.io/github/openwisp/netengine.
I left one minor request regarding link 404 verification, see below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍🏼
Deferring merge to @nemesisdesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@purhan LGTM!
I don't think squashing everything in one commit will be good, can you squash commits so we have one commit closing each issue mentioned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 👍
Closes #50, closes #49, closes #52, closes #55, closes #61