Skip to content
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

Addressed community review #1

Merged
merged 4 commits into from
Nov 5, 2021
Merged

Addressed community review #1

merged 4 commits into from
Nov 5, 2021

Conversation

sborkows
Copy link
Owner

  • Added short (-pmm) arg option
  • Provided some description in README.md
  • Added default value for packet manipulation module in generic packet
    module
  • Removed scapy from requirements.txt as it is now optional, if custom
    packet manipulation module is provided

- Added short (-pmm) arg option
- Provided some description in README.md
- Added default value for packet manipulation module in generic packet
  module
- Removed scapy from requirements.txt as it is now optional, if custom
  packet manipulation module is provided
@sborkows sborkows changed the title Addressed review. Addressed community review Oct 28, 2021
requirements.txt Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
## Pluggable packet manipulation module

By default, `ptf` uses `Scapy` as a packet manipulation module, but now it
can operate on other, custom one. To use other packet manipulation module,
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove custom?

Copy link
Owner Author

Choose a reason for hiding this comment

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

on another one ??? It sounds kind of general.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another thing is that this is a README file not release notes - this should describe current state not diff.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is inside of section New features, but I can adjust this section to be more as a current state 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if somebody removes features from New features section :)

README.md Outdated
one needs to provide it as an argument `-pmm` or
`--packet-manipulation-module` when running `ptf` binary.

In order to make it work, such module must implement methods, as defined in
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to describe or provide a minimal working example because now it's not easy to determine what needs to be implemented.

@sborkows sborkows merged commit 48977c0 into master Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants