Skip to content

Update Recommendations.RecommendedActions.TargetFrameworks field. #51

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ShanshanHe
Copy link

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.

The old `TargetFrameworks` schema is causing cta failures when parsing
the rules.

Also updated the indentation of a few recommendation files to be consistent with the rest.
@Eruanion
Copy link
Contributor

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?

Copy link
Contributor

@jonlouie jonlouie left a comment

Choose a reason for hiding this comment

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

Overall, the changes look good, but the schema validator needs to be updated to match the updated schema.

Suggested changes:

@jonlouie
Copy link
Contributor

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.

@ShanshanHe
Copy link
Author

ShanshanHe commented Apr 14, 2021

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.

@marknfawaz
Copy link
Contributor

*.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.

@ShanshanHe
Copy link
Author

ShanshanHe commented Apr 14, 2021

@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.

@ShanshanHe
Copy link
Author

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
Copy link
Contributor

@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.

@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 ShanshanHe requested review from huawenm and flywanli April 15, 2021 18:29
@ShanshanHe
Copy link
Author

ShanshanHe commented Apr 15, 2021

@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?

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.

4 participants