Skip to content
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

Ensure required property is present in generated documentation #602

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eduardomourar
Copy link
Contributor

@eduardomourar eduardomourar commented Oct 18, 2020

Issue #, if available:

Description of changes:

Whenever the required property was part of a combiner (e.g. anyOf) in a resource schema, the generated documentation would skip it. This change will ensure that required fields will be flattened and then used by the documentation generator.

You can check a documentation example here where the required property is ignored.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@PatMyron
Copy link
Contributor

PatMyron commented Dec 29, 2020

You can check a documentation example here where the required property is ignored.

Not seeing any differences after this change? None of the properties look like they're required by themselves anyways

Flattened some resource schemas in PRs linked in this issue that should have been better examples for testing, but not seeing changes from this for those previously unflattened schemas either

@eduardomourar
Copy link
Contributor Author

You can find an example here that I ran with this PR's branch. The fields are required true now instead false.

@PatMyron
Copy link
Contributor

You can find an example here that I ran with this PR's branch. The fields are required true now instead false.

I'd argue only RepositoryNames is required there. The other properties are only conditionally required

@iann0036
Copy link
Contributor

iann0036 commented Dec 29, 2020

For reference, the official resource docs use Conditional as an explicit state when representing conditionally required (i.e. anyOf) properties:

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-as-scheduledaction.html#cfn-as-scheduledaction-maxsize

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-s3-bucket-lifecycleconfig-rule.html#cfn-s3-bucket-rule-abortincompletemultipartupload

@eduardomourar
Copy link
Contributor Author

I agree that putting Conditional is the right way, but flattening will not be enough for that logic.

@cmgorton cmgorton added researching documentation I'll give you a guess what this is about. labels Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation I'll give you a guess what this is about. researching
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants