rdblue commented on code in PR #9940:
URL: https://github.com/apache/iceberg/pull/9940#discussion_r1645193788
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -61,6 +61,14 @@ security:
- OAuth2: [catalog]
- BearerAuth: []
+tags:
Review Comment:
@nastra, looks like this still needs to be addressed.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -61,6 +61,14 @@ security:
- OAuth2: [catalog]
- BearerAuth: []
+tags:
Review Comment:
@nastra, looks like this still needs to be addressed.
I think we also need to remove the Catalog API, Configuration API, and
OAuth2 API tags, since they are not specified here. We should probably add a
new oauth2 tag for the tokens endpoint. Any others that we want to fix?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -80,12 +95,14 @@ paths:
"
All REST clients should first call this route to get catalog
configuration
properties from the server to configure the catalog and its HTTP
client.
- Configuration from the server consists of two sets of key/value pairs.
+ Configuration from the server consists of:
Review Comment:
@nastra, should we update the 200 response to document that these are all
optional?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1417,6 +1443,35 @@ paths:
5XX:
$ref: '#/components/responses/ServerErrorResponse'
+ /v1/aws/s3/sign:
Review Comment:
@nastra, I think it would be better to merge the specs _after_ this PR.
There's already a lot changing and we don't need to move the other spec here at
the same time.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -80,12 +95,14 @@ paths:
"
All REST clients should first call this route to get catalog
configuration
properties from the server to configure the catalog and its HTTP
client.
- Configuration from the server consists of two sets of key/value pairs.
+ Configuration from the server consists of:
Review Comment:
@nastra, should we update the 200 response to document that these are all
optional?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1559,6 +1578,22 @@ components:
type: string
description:
Properties that should be used as default configuration; applied
before client configuration.
+ capabilities:
+ type: array
+ uniqueItems: true
+ items:
+ type: string
+ enum:
Review Comment:
I agree with @snazy here. Even if we don't recommend it, generated code
could attempt to validate the enum and break if a service implements a newer
version than a client. We should make this a list of strings and document that
it corresponds to tags in the spec in a description.
##########
aws/src/main/resources/s3-signer-open-api.yaml:
##########
@@ -56,13 +56,18 @@ servers:
description: Optional prefix to be appended to all routes
default: ""
+tags:
+ - name: remote-signing
+ description: Requires server to support remote signing
Review Comment:
I commented elsewhere. It makes sense to merge them to me, but let's not do
it in this PR. We can take care of it in a follow up.
##########
open-api/rest-catalog-open-api.py:
##########
@@ -54,6 +54,24 @@ class CatalogConfig(BaseModel):
...,
description='Properties that should be used as default configuration;
applied before client configuration.',
)
+ capabilities: Optional[
+ List[
+ Literal[
+ 'pagination',
+ 'scan-planning',
+ 'views',
+ 'vended-credentials',
+ 'remote-signing',
+ 'oauth2',
+ 'sigv4',
Review Comment:
@nastra, looks like we are missing `register-table` still?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
