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

Add a WFP provider object. #2282

Merged
merged 3 commits into from
Apr 7, 2023
Merged

Conversation

shankarseal
Copy link
Collaborator

@shankarseal shankarseal commented Apr 6, 2023

Description

Windows HLK tests for WFP requires drivers to add a provider object to identify the vendor. The changes were to add the provider to pass the HLK tests.

Testing

CI.

Documentation

Not required.

netebpfext/user/fwp_um.h Outdated Show resolved Hide resolved
netebpfext/user/fwp_um.h Outdated Show resolved Hide resolved
netebpfext/net_ebpf_ext.c Show resolved Hide resolved
netebpfext/net_ebpf_ext.c Show resolved Hide resolved
netebpfext/net_ebpf_ext.c Show resolved Hide resolved
netebpfext/net_ebpf_ext.c Show resolved Hide resolved
netebpfext/net_ebpf_ext.c Show resolved Hide resolved
netebpfext/net_ebpf_ext.c Show resolved Hide resolved
netebpfext/net_ebpf_ext.c Show resolved Hide resolved
@dthaler
Copy link
Collaborator

dthaler commented Apr 6, 2023

Testing

No.

Documentation

No.

https://github.com/microsoft/ebpf-for-windows/blob/main/CONTRIBUTING.md#contributing-code says:

For all new Pull Requests the following rules apply:

  • Existing tests should continue to pass.
  • Tests need to be provided for every bug/feature that is completed.
  • Documentation needs to be provided for every feature that is end-user visible.

so saying "Testing: No" is not allowed. You need to at minimum say it's covered by existing tests (this isn't), or by manual testing or whatever. The description mentions HLK tests so I would expect the Testing section to talk about that. But we really want tests to be part of CI/CD, so either you should argue that HLK tests can be added to the github workflow, or else you should add a unit test that covers this functionality.

Copy link
Collaborator

@dthaler dthaler left a comment

Choose a reason for hiding this comment

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

See comment above.

Co-authored-by: Anurag Saxena <43585259+saxena-anurag@users.noreply.github.com>
@shankarseal
Copy link
Collaborator Author

Testing

No.

Documentation

No.

https://github.com/microsoft/ebpf-for-windows/blob/main/CONTRIBUTING.md#contributing-code says:

For all new Pull Requests the following rules apply:

  • Existing tests should continue to pass.
  • Tests need to be provided for every bug/feature that is completed.
  • Documentation needs to be provided for every feature that is end-user visible.

so saying "Testing: No" is not allowed. You need to at minimum say it's covered by existing tests (this isn't), or by manual testing or whatever. The description mentions HLK tests so I would expect the Testing section to talk about that. But we really want tests to be part of CI/CD, so either you should argue that HLK tests can be added to the github workflow, or else you should add a unit test that covers this functionality.

I wanted to write "N/A" - not sure why I wrote the rule "No". Sorry about that.

@dthaler
Copy link
Collaborator

dthaler commented Apr 7, 2023

I wanted to write "N/A" - not sure why I wrote the rule "No". Sorry about that.

Testing should never say "N/A". It's always applicable. For things like doc-only changes "No impact." would be the answer, but this is not a doc-only change.

@shankarseal
Copy link
Collaborator Author

I wanted to write "N/A" - not sure why I wrote the rule "No". Sorry about that.

Testing should never say "N/A". It's always applicable. For things like doc-only changes "No impact." would be the answer, but this is not a doc-only change.

Done. please approve.

@shankarseal shankarseal added this pull request to the merge queue Apr 7, 2023
Merged via the queue into microsoft:main with commit 5d32a5b Apr 7, 2023
67 checks passed
@shankarseal shankarseal deleted the wfp_provider branch April 7, 2023 22:49
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.

None yet

4 participants