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

EndpointManagerTest "AddHostsFileEntry" does not work as intended #135

Closed
Eneuman opened this issue Dec 27, 2022 · 3 comments · Fixed by #153
Closed

EndpointManagerTest "AddHostsFileEntry" does not work as intended #135

Eneuman opened this issue Dec 27, 2022 · 3 comments · Fixed by #153
Labels
enhancement New feature or request

Comments

@Eneuman
Copy link
Contributor

Eneuman commented Dec 27, 2022

In this line it says that EndpointManagerRequest<T> where T is a IEnumerable<HostsFileEntry>.

var request = new EndpointManagerRequest<IEnumerable<HostsFileEntry>>()

Looking at the code that this test should cover:

var addHostsFileEntryRequest = JsonHelpers.DeserializeObject<EndpointManagerRequest<(string workloadNamespace, IEnumerable<HostsFileEntry> entries)>>(request);

T should be a tuplet of (string workloadNamespace, IEnumerable<HostsFileEntry> entries).
Yet, this test passes.

When switching to System.Text.Json this test failes.

@Eneuman
Copy link
Contributor Author

Eneuman commented Dec 27, 2022

Since System.Text.Json is not very good att handling Tuplets (alot of extra work will be needed with custom convert etc), I would suggest replacing the tuplet with a new "EndpointManagerRequestArgument" and then create separate objects for each argument type. That way you both get strongly typed objects and you can set a constraint on what can be send to the EndPointManager.

@hsubramanianaks hsubramanianaks added the enhancement New feature or request label Dec 27, 2022
@elenavillamil
Copy link
Contributor

The suggestion sounds good. We will discuss with the team on our next weekly sync and update here. Thank you very much for contributing to make bridge better!

@hsubramanianaks
Copy link
Collaborator

This is handled in this huge PR - #153.

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

Successfully merging a pull request may close this issue.

3 participants