netgrasp icon indicating copy to clipboard operation
netgrasp copied to clipboard

issue #8 perform local mac lookups

Open natej opened this issue 7 years ago • 7 comments

Sorry for all the changes. I know this a rather large patch. Some of the changes aren't directly related to the enhancement, but most of them were necessary to allow submitting something worthwhile. There was at least one circular reference I had to workaround.

See new module utils/mac.py and function lookup_local(). Mac matching needs to be added here. It's commented and some placeholder code is there. Other than that function, all functionality should be working.

You may want to test with your email/templates to make sure I didn't break anything in there. I tested things, but I don't have email or notifications setup, so I won't see breakage there.

  • Added mac module in utils.
  • See TEST vars at top to assist with dev/debugging with local data.
  • Use wireshark manuf file for db import.
  • Some unrelated bug fixes.
  • General cleanups along with refactoring for consistency.
  • Use print function for prep for python 3.

natej avatar Jul 24 '17 06:07 natej

Sorry if I messed up your commit message/history on your last commit for time_ago. I was trying to cleanup an initial (phase 1) commit after I merged in upstream and I don't think I got it quite right. I'm trying to keep up with your master and merge in changes so it's easy for you to deal with.

natej avatar Jul 24 '17 06:07 natej

Let me know if something doesn't seem right. I tried to squash the commits into one, but I got your last commit in there. I can delete this PR and re-do it. I saved it before the rebase -i.

natej avatar Jul 24 '17 06:07 natej

Sorry for all the changes. I know this a rather large patch. Some of the changes aren't directly related to the enhancement, but most of them were necessary to allow submitting something worthwhile.

Alas, I disagree. There's a lot of style changes that seem totally unnecessary. I believe this PR can be much more specific to the change it's trying to make, and is getting distracted in unrelated/unnecessary changes. Let me know if you want help splitting it.

There was at least one circular reference I had to workaround.

I apparently missed this in all the changes: where was this? Perhaps we can fix that first in its own PR?

jeremyandrews avatar Jul 24 '17 07:07 jeremyandrews

Let me know if something doesn't seem right. I tried to squash the commits into one, but I got your last commit in there.

I don't see any need to squash your commits; I'm fine preserving all history. Especially when the diff is so huge, it can be helpful to go back through the history and figure out how/why you got here. That said, I far prefer a more directed PR that's specific to the functional change you're introducing.

jeremyandrews avatar Jul 24 '17 07:07 jeremyandrews

That's ok, no apology needed. That's my mistake for sending a large patch. I should've made several commits.

The enhancement is really contained only in utils/mac.py and a small function at the bottom of cli.py.

The blocker was the database class. I wanted to use what you'd already written (get/set_state). It tries to import netgrasp, so there was a circular import if you try to create a Database instance early in netgrasp's life-cycle, e.g. from _load_configuration().

database.Database() seems like more of a stand-alone class that shouldn't have any external dependencies when creating an instance. So anything it needs should be passed-in via args.

Thanks for your feedback and taking the time to comment on all the code.

natej avatar Jul 24 '17 15:07 natej

@natej hi, do you plan to rework this PR? I'll likely rework it myself soon, based on my feedback, if not.

jeremyandrews avatar Aug 13 '17 07:08 jeremyandrews

Hi,

I haven't had time to work on any changes. That would be great if you could use some of it. I hope it at least gives you a good start.

I look forward to seeing what you do with it. I definitely think it's a good feature to have. :)

Thanks for checking.

natej avatar Aug 13 '17 10:08 natej