-
Notifications
You must be signed in to change notification settings - Fork 3k
SPEC - Update REST Catalog spec to put examples + type details on catalog config #4186
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
SPEC - Update REST Catalog spec to put examples + type details on catalog config #4186
Conversation
|
cc @rdblue. I was working on this PR for This isn't doesn't change the spec at all. I'll also update the response to match the name |
|
This passes based on editor.swagger.io and openapi-schema-validator $ openapi-spec-validator docs/rest/rest-catalog-open-api.yaml
OK |
f063af3 to
e059033
Compare
| } | ||
| } | ||
| } | ||
| $ref: '#/components/responses/RESTCatalogConfigResponse' |
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 think it is clear that all the request/response objects are for REST, so we can probably omit that from the example and object names.
| example: 404 | ||
|
|
||
| CatalogConfiguration: | ||
| RESTCatalogConfig: |
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.
Why this rename?
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 matches the values that are used. Although now we just have RESTCatalogConfigResponse.
e059033 to
ce7e6c2
Compare
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/RESTCatalogConfig' |
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.
Will probably update to inline this and get rid of the RESTCatalogConfig object.
| properties: | ||
| overrides: | ||
| type: object | ||
| items: |
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 this valid? I thought it additionalProperties was used to set the type of values in a map/object.
| description: | ||
| Properties that should be used to override client configuration; applied after defaults and client configuration. | ||
| example: { "warehouse": "s3://bucket/warehouse" } | ||
| default: { } |
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.
We don't use defaults elsewhere. is this needed?
|
|
||
| RESTCatalogConfigResponse: | ||
| description: | ||
| Configuration from the server consists of two sets of key/value pairs. |
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 thought that you needed 2 newlines between paragraphs, and that * didn't work for bulleted lists.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |

The REST catalog receives some of its configuration from the server.
This updates the REST Catalog spec to show:
overridesandpropertiesis a string-to-string map (theadditionalProperties: { type: string }indicates this).This does not change anything about the spec, just provides more detail.
See also #4184 which adds in a response object for this type.