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.

Sign up for GitHub

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

Jump to bottom

Skeleton NF Functionality Improvements #313

Merged
merged 15 commits into from
Aug 4, 2022
Merged

Skeleton NF Functionality Improvements #313

merged 15 commits into from
Aug 4, 2022

Conversation

jettjacobs
Copy link

Summary:

Changes Resolved From #312

Additional Functionality:

  • Additional Command-Line Arguments
  • Timer-based or Packet-based print delays

Usage:

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements
Bug fixes
New functionality X
New NF/onvm_mgr args
Changes to starting NFs X
Dependency updates
Web stats updates

Merging notes:

  • Dependencies: None

TODO before merging :

  • PR is ready for review

Test Plan:

  • Run NF on the manager
  • Use speed_tester to send packets to skeleton
  • Ensure that the skeleton NF can correctly print the number of packets. Test timer-based (-p) delays and packet-based delays (-v).

Review:

Review comments to confirm validity
Command-line arguments should be mutually exclusive

@dennisafa
Copy link
Member

Did you want to close this PR and make your updates as a new commit to your previous PR? Otherwise, could you close the other PR out?

@jettjacobs
Copy link
Author

@dennisafa I will close out the other PR, thanks! I created a new PR to pull from my own forked develop branch, rather than master.

dennisafa
dennisafa previously approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@dennisafa dennisafa left a comment

Choose a reason for hiding this comment

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

LGTM, just resolve conflicts.

@jettjacobs
Copy link
Author

@dennisafa sorry about dismissing the review, I had to make one more adjustment. Merge conflicts are now resolved

@twood02 twood02 linked an issue Nov 19, 2021 that may be closed by this pull request
Simple Skeleton #309
Open
twood02
twood02 approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@twood02 twood02 left a comment

Choose a reason for hiding this comment

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

lgtm

@twood02
Copy link
Member

twood02 commented Jul 6, 2022

@catherinemeadows sometime when I am not around, could you do a meeting where you show the summer students how you would test and eventually approve this PR? That would be good for them to see.

@catherinemeadows
Copy link
Contributor

@catherinemeadows sometime when I am not around, could you do a meeting where you show the summer students how you would test and eventually approve this PR? That would be good for them to see.

Yep, will do!

@twood02 twood02 merged commit f5ef56c into sdnfv:develop Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twood02 twood02 twood02 approved these changes

@dennisafa dennisafa dennisafa left review comments

Assignees
No one assigned
Labels
new NF 🛰️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Simple Skeleton
4 participants
@jettjacobs @dennisafa @twood02 @catherinemeadows