search

joelagnel / bpfd

BPFd (Deprecated, please see README.md) : Berkeley Packet Filter daemon (BPFd). Makes it possible to run BCC tools across systems.
Apache License 2.0
95 stars 23 forks source link

Restructured the directory structure to be clearer and more meaningful #23

Closed jcanseco closed 6 years ago

joelagnel commented 6 years ago

Could you please call the scripts as "scripts/setup-scripts". The original structure had the word setup in it. Could you please change 'bcc' to bcc-deps so it's clear that these are dependencies pulled externally. Also please include a README in bcc-deps which clearly mentions which bcc commit do these files come from. In the README, mention that these files should not to be modified directly but rather pulled from BCC project. And that a script is in the works to pull it. Basically it should be clear what the purpose it. Lastly I think bcc-deps shouldn't share the src/ tree structure with bpfd since we are also going to be syncing bpfd into bcc right ? Sharing the same tree might complicate that. So I suggest a structure like: bcc-deps/ src/ scripts/setup-scripts/ Its also just cleaner this way.

On Mar 5, 2018 4:34 PM, "Jazel Canseco" notifications@github.com wrote:

You can view, comment on, or merge this pull request online at:

https://github.com/joelagnel/bpfd/pull/23 Commit Summary

  • Move scripts outside the src directory since they are not needed to
  • Rename the directory containing the source files taken from BCC to 'bcc'

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/joelagnel/bpfd/pull/23, or mute the thread https://github.com/notifications/unsubscribe-auth/AACSVO0om5AqJQuNGyE_uWugWzNSVq3Nks5tbdmkgaJpZM4Sd5-C .

jcanseco commented 6 years ago

I made the requested changes. Please take a look. I'll let you review it first before I propagate these changes into the other PRs.

joelagnel commented 6 years ago

Ok. At the moment I am out of time since I am traveling. I will try to get to it tomorrow and review the new changes.

joelagnel commented 6 years ago

You added more commits ontop of old commits. Make sure the PR has only commits that you want merged. Use git rebase. Right now the PR shows 6 commits which seems wrong.

jcanseco commented 6 years ago

Okay. I cleaned up the branch using some rebase magic.

I'll also do it this way for requested changes in future PRs whenever appropriate.

joelagnel commented 6 years ago

For future, lets make sure commit messages are formatted properly. In one of the commits, the title and body are split. Also lets make them more descriptive. Someone reading the history should be clear what the changes do without having to refer to the PR description in github