-
Notifications
You must be signed in to change notification settings - Fork 59
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
Resolves Warning SYSLIB021 - Derived cryptographic types are obsolete #210
Conversation
It would be great if we could also add a tests for Mono. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Eneuman Thanks for the changes, requested small changes.
@Eneuman Thanks for your PR! My review of this pull request is as follows: This pull request looks good overall. It resolves the SYSLIB021 warning and introduces tests for the MacAddressHash functionality. The changes look good. The new tests are comprehensive and cover the MacAddressHash functionality. The changes to FipsCompliantSha.cs and MACInformationProvider.cs look correct and are in line with the changes proposed. The only suggestion I have is to add a comment to the RunCommandAndGetOutput method in MACInformationProvider.cs to explain why the timeout is set to 30 seconds. This will help other developers understand the purpose of the timeout and why it is set to 30 seconds. |
@Eneuman if you have WSL2 you can run this command |
@Eneuman Thanks for your PR! This pull request looks good overall. It resolves the SYSLIB021 warning and introduces tests for the MacAddressHash functionality. The RunCommandAndGetOutput function was changed to use the existing The only suggestion I have is to add more comments to the code to explain the purpose of the changes. This will help other developers understand the code better and make it easier to maintain in the future. |
here is the sample output for
|
@hsubramanianaks I have another suggestion, why dont we just replace this code with .Net native plattform independent code like this?
I switched to MD5 because it's the fastest algorithm, and we are not doing encryption here ;) |
yes if it make sense to do in this PR please go ahead. |
@hsubramanianaks I'll make a seperate PR for this :) This PR can just solve the warning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☕️🙏 LGTM - Hopefully we got this all tested et. al. from internal channels. thanks heaps @hsubramanianaks and @Eneuman 🎊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testing team approved these changes.
This PR solves the SYSLIB021 - Derived cryptographic types are obsolete warning.
It also introduces tests for the MacAddressHash functionallity.
The RunCommandAndGetOutput function was changed to use the existing
_platform.ExecuteAndReturnOutput
to align the way the program executes externa processes. The timeout was set to 30 sec to make sure the command have enought time to finish. The process will like finish within a second under normal conditions.