Skip to content

Added ability to include NRPT records to NameServers list automatical…#114

Merged
MichaConrad merged 4 commits intoMichaCo:devfrom
SteveSyfuhs:feature/add-nrpt-ns
May 17, 2021
Merged

Added ability to include NRPT records to NameServers list automatical…#114
MichaConrad merged 4 commits intoMichaCo:devfrom
SteveSyfuhs:feature/add-nrpt-ns

Conversation

@SteveSyfuhs
Copy link
Copy Markdown
Contributor

…ly on Windows

Copy link
Copy Markdown
Collaborator

@MichaConrad MichaConrad left a comment

Choose a reason for hiding this comment

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

Looks pretty good overall, but I think we can simplify it by removing the configuration flags / options.

Comment thread src/DnsClient/DnsQueryOptions.cs Outdated
Comment thread src/DnsClient/Interop/Windows/NameResolutionPolicy.cs Outdated
Comment thread src/DnsClient/Interop/Windows/NameResolutionPolicy.cs Outdated
Comment thread src/DnsClient/Interop/Windows/NameResolutionPolicy.cs
Comment thread src/DnsClient/NameServer.cs
Comment thread src/DnsClient/NameServer.cs Outdated
@MichaConrad MichaConrad linked an issue May 16, 2021 that may be closed by this pull request
@MichaConrad
Copy link
Copy Markdown
Collaborator

I actually didn't know about that new rule in .NET5
https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1416

Or those new attributes... Seems fancy
The new construct OperatingSystem only exists in .NET5 which means we cannot use it anyways.
I think you can safely disable the rule in .editorconfig

@MichaConrad
Copy link
Copy Markdown
Collaborator

never mind, you fixed it already ;)

@SteveSyfuhs SteveSyfuhs requested a review from MichaConrad May 17, 2021 16:28
@SteveSyfuhs
Copy link
Copy Markdown
Contributor Author

Yeah, this rule is new to me too. Was easy enough to fix though.

Copy link
Copy Markdown
Collaborator

@MichaConrad MichaConrad left a comment

Choose a reason for hiding this comment

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

Looks good, I tested it also locally on Windows and Linux, seems all fine.

Thanks a lot again for the contribution @SteveSyfuhs !

@MichaConrad MichaConrad merged commit d6015b2 into MichaCo:dev May 17, 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.

Split tunnel resolution fails

2 participants