porting-assistant-dotnet-datastore
porting-assistant-dotnet-datastore copied to clipboard
Update Recommendations.RecommendedActions.TargetFrameworks field.
The old TargetFrameworks schema is causing cta failures when parsing the rule files.
Also updated the indentation of a few recommendation files to be consistent with the rest.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
I think we can ignore all the files with recommendation in their names as those were automatically added I believe and we have to revisit them at a later date anyway and they contain no rules. Some of the antlr rules file were heavily modified also, with a lot of redactions was that necessary to ensure they match the schema?
I think we can ignore all the files with recommendation in their names as those were automatically added I believe and we have to revisit them at a later date anyway and they contain no rules. Some of the antlr rules file were heavily modified also, with a lot of redactions was that necessary to ensure they match the schema?
Jonathan made some good points... Not sure if the *recommendation.json files need to be updated... in which case the validator does not need to be updated.
@Eruanion The heavy changes to the antlr rules only appear so due to changes in indentation. The content looks unchanged otherwise.
I think we can ignore all the files with recommendation in their names as those were automatically added I believe and we have to revisit them at a later date anyway and they contain no rules.
These rule files are causing cta to fail though if we run it with rule-dir specified as the porting-assistant-dotnet-datastore/recommendation directory. Do you know how they are added automatically? maybe we could modify the automation part to have the schema updated.
Some of the antlr rules file were heavily modified also, with a lot of redactions was that necessary to ensure they match the schema?
Yeah these files are indented with 2 spaces as the rest of them are 4 spaces, the content of the files are not changed.
*.recommendation files shouldn't run in CTA, they don't have actions. We can modify each file if we have actions. To modify these files, we need to modify the code generating them since these are autogenerated.
@marknfawaz Thank you for the explanation. Good to know the recommendation.json are not used by cta. Could you point me to the location that generates the recommendation files pls. If the recommendation.json files are not used in cta, how are they consumed?
My only concern is the inconsistency of the TargetFrameworks schema between these recommendation.json files and the rest of the rule files which might bite us in the future. I could revert my changes, or update the code that generates the recommendation files, please advise which would be better to proceed.
Overall, the changes look good, but the schema validator needs to be updated to match the updated schema.
* The schema validator is run as a GitHub action which we can see has failed [here](https://github.com/aws/porting-assistant-dotnet-datastore/pull/51/checks?check_run_id=2347491182) * [Code for the schema validator](https://github.com/aws/porting-assistant-dotnet-datastore/tree/master/RecommendationValidator)Suggested changes:
* [This property](https://github.com/aws/porting-assistant-dotnet-datastore/blob/c95b76793204864197ffe2e01981da3301cee9c3/RecommendationValidator/RecommendationValidator/RecommendationPOJO.cs#L94) needs to be updated to match [the schema in CTA](https://github.com/aws/cta/blob/3a44113d2e9447a577f7cd92669b4f36e67d67e1/src/CTA.Rules.Models/NamespaceRecommendations.cs#L43-L65) * Rename `RecommendationPOJO` to `RecommendationPOCO` to follow C# naming convention
Great catch @jonlouie , thanks for sharing the detailed information, if we do proceed with updating the recommendation.json files, I will include an update to the Validator
@marknfawaz Thank you for the explanation. Good to know the
recommendation.jsonare not used by cta. Could you point me to the location that generates the recommendation files pls. If therecommendation.jsonfiles are not used in cta, how are they consumed?My only concern is the inconsistency of the
TargetFrameworksschema between theserecommendation.jsonfiles and the rest of the rule files which might bite us in the future. I could revert my changes, or update the code that generates the recommendation files, please advise which would be better to proceed.
@ShanshanHe These files are not relevant to CTA, but I don't see an issue with updating them. You can check with Andrew or Huawen on how they are being created. As for the change itself, while I agree it's good to keep all files in sync, we'll have to see how complex updating the file generation and validation process is then decide.
@ShanshanHe These files are not relevant to CTA, but I don't see an issue with updating them. You can check with Andrew or Huawen on how they are being created. As for the change itself, while I agree it's good to keep all files in sync, we'll have to see how complex updating the file generation and validation process is then decide.
Just had a chat with @flywanli and @huawenm , they are good with updating the schema. @huawenm has pointed me to the file generation code, looks like it'll be a very small change. And I have the validator updated as well in my local branch. Should I submit the change?