Skip to content

Conversation

aniket-okta
Copy link
Contributor

This pull request addresses an issue related to the use of RuntimeInformation.FrameworkDescription in the UserAgentHelper class of the Okta.AspNet.Abstractions library. The change replaces the dependency on RuntimeInformation with Environment.Version for projects targeting .NET 5 and above. The goal is to reduce unnecessary dependencies, especially in modern .NET versions, by utilizing Environment.Version to obtain framework version details.

Changes Made:

  • Replaced the usage of RuntimeInformation.FrameworkDescription with Environment.Version.ToString() for .NET 8+ versions.
  • Used conditional compilation to handle differences between .NET 8 and older versions.
  • Updated the logic in UserAgentHelper to ensure proper fallback behavior based on the runtime framework version.
  • Added integration test to verify the correct assignment of the framework description across different .NET versions.

Testing Done:

  • All unit tests were re-run to ensure existing functionality was preserved.
  • Added a new integration test to verify the functionality of the frameworkDescription logic.
  • Verified that the framework description is correctly retrieved for both .NET 8+ and earlier versions.

Related Issues:
-#274

Copy link
Collaborator

@laura-rodriguez laura-rodriguez left a comment

Choose a reason for hiding this comment

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

Let's revisit the UT and ensure the new and existing functionality is being tested properly.

@aniket-okta
Copy link
Contributor Author

Thanks for the feedback, Laura. I've updated the test to directly call UserAgentHelper.GetFrameworkDescription. The part (".NET Core foo", "foo/Microsoft.NETCore.App") wasn't necessary, as my goal was to test the FrameworkDescription based on .NET versions. The updated test now achieves that. I also ensured that all other tests are passing to confirm that the existing functionality is intact. Please let me know if you think any further changes are needed.

@aniket-okta aniket-okta force-pushed the okta-838697-UpdateFrameworkDescriptionHandling branch from 51614ab to ffd8b44 Compare January 8, 2025 07:35
@aniket-okta aniket-okta force-pushed the okta-838697-UpdateFrameworkDescriptionHandling branch from ffd8b44 to a80050f Compare January 8, 2025 07:36
Copy link
Collaborator

@laura-rodriguez laura-rodriguez left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thank you for making these changes, @aniket-okta!

The next step will be releasing a patch and updating the abstractions dependency in both ASP.NET SDKs 🚀

@aniket-okta aniket-okta merged commit 78c49e7 into master Jan 13, 2025
1 check passed
@aniket-okta aniket-okta deleted the okta-838697-UpdateFrameworkDescriptionHandling branch January 13, 2025 14:50
@aniket-okta aniket-okta mentioned this pull request Jan 16, 2025
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.

2 participants