-
Notifications
You must be signed in to change notification settings - Fork 4k
Prediction/Commands Service API v2 updates #13767
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
Conversation
Thank you for your contribution jjaguirre394! We will review the pull request and get back to you soon. |
tools/Az.Tools.Predictor/Az.Tools.Predictor.Test/Mocks/MockAzPredictorService.cs
Outdated
Show resolved
Hide resolved
// limitations under the License. | ||
// ---------------------------------------------------------------------------------- | ||
|
||
using Newtonsoft.Json; |
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.
Can you convert to use the built-in json serialization https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-how-to?pivots=dotnet-5-0
You can find the default option for deserialization here https://github.com/Azure/azure-powershell/blob/master/tools/Az.Tools.Predictor/Az.Tools.Predictor/Utilities/JsonUtilities.cs
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.
Same to other files.
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.
Is there a reason we use the built-in json serialization over the Newtonsoft one? I ask because it seems the built-in version wasn't able to parse the string into the objects I had defined. I'm sure it is possible but I found that the Newtonsoft library was able to do it without any issue. If we really don't want to have a dependency on this library, I can find out how to do this using the built-in version.
tools/Az.Tools.Predictor/Az.Tools.Predictor/AzPredictorService.cs
Outdated
Show resolved
Hide resolved
tools/Az.Tools.Predictor/Az.Tools.Predictor/AzPredictorService.cs
Outdated
Show resolved
Hide resolved
tools/Az.Tools.Predictor/Az.Tools.Predictor/AzPredictorService.cs
Outdated
Show resolved
Hide resolved
tools/Az.Tools.Predictor/Az.Tools.Predictor/CommandLinePredictor.cs
Outdated
Show resolved
Hide resolved
tools/Az.Tools.Predictor/Az.Tools.Predictor/PredictiveCommand.cs
Outdated
Show resolved
Hide resolved
tools/Az.Tools.Predictor/Az.Tools.Predictor/AzPredictorService.cs
Outdated
Show resolved
Hide resolved
tools/Az.Tools.Predictor/Az.Tools.Predictor/PredictiveCommand.cs
Outdated
Show resolved
Hide resolved
- Removed unnecessary usings - Added copyright - Misc. code clean up
Description
The Aladdin service is releasing v2 API endpoints to allow for version support of the Predictions and Commands endpoints. This would allow this Powershell client to query the model for predictions based on the module version.
Checklist
CONTRIBUTING.md
ChangeLog.md
file(s) has been updated:ChangeLog.md
file can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
## Upcoming Release
header -- no new version header should be added