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

Generic #597

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Generic #597

wants to merge 2 commits into from

Conversation

elazarg
Copy link
Member

@elazarg elazarg commented May 23, 2024

This PR demonstrates the changes required in order to use the generic data structures (and ICMP set) in np-guard/models#33. 95% of the changes are renaming, but some are related to the introduction of a dedicated ICMP set.

Some tests still do not pass, due to an issue with tracking statefulness (which IMO should be part of this repo and not np-guard/models).

The line replace github.com/np-guard/models => ../models in go.mod is useful for local experimentation until np-guard/models#33 is merged.

adisos

This comment was marked as off-topic.

Signed-off-by: Elazar Gershuni <[email protected]>
@elazarg
Copy link
Member Author

elazarg commented Jun 19, 2024

Updated to accommodate #634.
The tests cannot pass in github. Locally:

  • Some tests are updated to explicitly state the only code in ICMP types that have only single code.
  • Two tests fail due to what seems like an inconsistent ordering of partitions. It might be a good idea to use ordered maps instead of hash maps.

Signed-off-by: Elazar Gershuni <[email protected]>
@elazarg
Copy link
Member Author

elazarg commented Jun 20, 2024

Okay, now all the tests pass. The issue was the use of CartesianRightTriple instead of CartesianLeftTriple when partitioning TCPUDPSet. The decision is currently at the library side, but it should probably move to the client side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants