Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[bitnami/kubeapps] Add nullable annotation in a param #9209
[bitnami/kubeapps] Add nullable annotation in a param #9209
Changes from 3 commits
ff3fe49
5ea891b
28886ea
f72bfdf
1876ae0
61aa47a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If this field is a string, you would need to add:
in order to show type string into the schema
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.
This param can be:
So, I'm not sure we would want to show
type: string
in the schema, perhapsThere 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.
sure, then you should add
array
where I putstring
. You assumption is correct, just do not add spaces (I am not sure right now if there is a trim into the code for thatThere 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.
I've added it, but it results in an invalid openapi schema: the required
items
property for arrays is not being generated.With
@param .... [nullable]
, valid schema, but not representing the realityWith
@param .... [array, nullable]
, invalid schemaExpected valid OpenAPI 3.0.X schema:
Note that we are explicitly not targeting openapi 3.1, otherwise, the
nullable
prop would be removed, and thetype
object would become an array. Useful SO post clarifying the differences.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.
So, rather than generating an invalid schema, I'd go with leaving it as is, until the
items
can be added/inferred in this edge case. WDYT?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.
Looks like another corner case for arrays. Thank you for sharing it.
Currently, if you don't specify any modifier for setting the type, it will be object by default. I think if you add just
[nullable]
thetype
will be set toobject
into the schema. Is that what you mean?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.
Yes, as we are not using the schema currently, semantic correctness is not a priority, so having
type: object
is something bearable until this edge case gets sorted out.I mean, sacrificing semantic over syntactic correctness is preferred in this case.
Do you want me to file an issue in the readme's repo? Can we merge this PR then?
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.
As discussed offline, this corner case is going to be resolved soon in the readme-generator tool, therefore, let me update the PR to also include the [array] modifier.
Thanks!
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.
I agree, let's proceed with this PR, I will try to fix t