-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add OUI model #2896
Add OUI model #2896
Conversation
c5a0e39
to
c70b45f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2896 +/- ##
==========================================
+ Coverage 56.35% 56.36% +0.01%
==========================================
Files 603 604 +1
Lines 43862 43858 -4
Branches 48 48
==========================================
+ Hits 24719 24722 +3
+ Misses 19131 19124 -7
Partials 12 12 ☔ View full report in Codecov by Sentry. |
076cde1
to
0aea33b
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.
I think you should consider the option to store the OUI prefix as a regular PostgreSQL macaddr
data type, which has several benefits when it comes to indexing, sorting, joining in pure SQL etc.
Just ensure that the last three bytes are always zero (this could also be accomplished by a constraint on the table. The built-in trunc
function can also truncate mac address values).
See https://www.postgresql.org/docs/13/datatype-net-types.html and https://www.postgresql.org/docs/13/functions-net.html
Made some changes according to your suggestions |
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.
Looks ok to me. We would usually put the model in nav.models.manage
, since the database object is placed in the manage
namespace - but we could really stand to split up this giant module anyway, so let's roll with it.
My only nitpick is the naming of the SQL change file itself. It should follow the established pattern of corresponding with the major.minor
of the NAV release we expect it to land in, so it should either be prefixed sc.05.10.
or sc.05.11.
...
53e0791
to
987d886
Compare
987d886
to
ec34f17
Compare
Co-authored-by: Johanna England <[email protected]>
Co-authored-by: Johanna England <[email protected]>
We got it super free this way with absolute minimal code and maximum speed. Better to do it with postgres code thats made to do this stuff super fast than manually with slow python code. The max length might not be necessary tbh since postgres MACADDR type should only allow a valid mac address anyways |
I noticed the comment by @lunkwill42 too late, which is why I hid this comment 😅 |
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.
Short and sweet!
Closes #2957