[PoC] [Do Not Merge] Make policy management APIs separate from IRC spec #856
Conversation
# Conflicts: # service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/IcebergErrorResponse' |
There was a problem hiding this comment.
I guess it is OK to reference object from another yaml file. Can you confirm?
There was a problem hiding this comment.
Yes it is possible. I did not do that here initially because there are several other places that reference the IcebergErrorResponse. So I define this in this yaml, but the definition is reference from another yaml file:
schemas:
Namespace:
$ref: '../rest-catalog-open-api.yaml#/components/schemas/Namespace'
IcebergErrorResponse:
$ref: '../rest-catalog-open-api.yaml#/components/schemas/IcebergErrorResponse'
| --- | ||
| openapi: 3.0.3 | ||
| info: | ||
| title: Apache Iceberg REST Catalog API |
There was a problem hiding this comment.
Nit: may change the tile a bit reflect its scope, which including both Iceberg and Polaris-native REST APIs
|
|
||
|
|
||
| components: | ||
| securitySchemes: |
There was a problem hiding this comment.
I assume we copy it from Iceberg rest spec yaml, as securitySchemes is NOT referenced by any endpoint. Can you confirm?
Is there a way to add a $ref for components?
There was a problem hiding this comment.
The 2 securitySchemes are used as global security settings above:
security:
- OAuth2: [catalog]
- BearerAuth: []
I just realized we can reference them like this.
components:
securitySchemes:
OAuth2:
$ref: './rest-catalog-open-api.yaml#/components/securitySchemes/OAuth2'
BearerAuth:
$ref: './rest-catalog-open-api.yaml#/components/securitySchemes/BearerAuth'
| servers: | ||
| - url: "{scheme}://{host}/{basePath}" | ||
| description: Server URL when the port can be inferred from the scheme | ||
| variables: | ||
| scheme: | ||
| description: The scheme of the URI, either http or https. | ||
| default: https | ||
| host: | ||
| description: The host address for the specified server | ||
| default: localhost | ||
| basePath: | ||
| description: Optional prefix to be appended to all routes | ||
| default: "" | ||
| - url: "{scheme}://{host}:{port}/{basePath}" | ||
| description: Generic base server URL, with all parts configurable | ||
| variables: | ||
| scheme: | ||
| description: The scheme of the URI, either http or https. | ||
| default: https | ||
| host: | ||
| description: The host address for the specified server | ||
| default: localhost | ||
| port: | ||
| description: The port used when addressing the host | ||
| default: "443" | ||
| basePath: | ||
| description: Optional prefix to be appended to all routes | ||
| default: "" | ||
| # All routes are currently configured using an Authorization header. | ||
| security: | ||
| - OAuth2: [catalog] | ||
| - BearerAuth: [] |
There was a problem hiding this comment.
I think it's worth to document any section copied from somewhere else. I assume this section is copied from Iceberg REST spec.
There was a problem hiding this comment.
Good point. I will add comments here.
|
@flyrain Thanks for reviewing this. I will update the PR and do some additional research on how to render a preview of APIs on website |
|
To render the combined spec in website, we can rely on tools like redocly to re-combine them into one spec file, so that the |
| /v1/{prefix}/namespaces/{namespace}/tables/{table}/notifications: | ||
| $ref: './rest-catalog-open-api.yaml#/paths/~1v1~1{prefix}~1namespaces~1{namespace}~1tables~1{table}~1notifications' |
There was a problem hiding this comment.
Maybe we could extract this endpoint first in this PR, and left out the policy related API to #808. WDYT?
There was a problem hiding this comment.
That sounds a good starting point to split spec. If you do not mind, I can open a new PR to do that and leave the current one as a PoC for policy management APIs. This way, we will always have a reference point to see how things look when adding more APIs to the spec.
There was a problem hiding this comment.
Created #906, I temporarily left it as draft because we need to figure out a proper way to update the website pointing to new spec file, otherwise it will break the website.
I think we did that before, but it is removed somehow. It makes sense to me to render them together as there are multiple cross-references. |
|
I thought we do not want to have two PRs #906 and #856 for the same thing (see #884 and https://lists.apache.org/thread/bcnh1dwgoxd2dvtqk6z935gfzmh4q0jq) |
This PR depends on #808, while #808 also depends on this one, creating a cyclic dependency. To resolve this, we have two options:
|
The titles of the PRs are the same so it's the same thing, or isn't it? |
HonahX
changed the title
HonahX
changed the title
|
Apologies for any confusion. I should have provided a clearer title. Allow me to clarify the intent of this PR: The current one, PR #856 was initially created as a proof of concept to facilitate discussions on this Apache mailing list thread regarding specification separation. It utilizes policy management APIs to demonstrate that PR #906, on the other hand, will specifically focus on separating Polaris-native APIs (currently limited to the notifications API) from the IRC specification. Additionally, it will include documentation and commands for merging YAML files into a single specification for website display. I will update the title and description accordingly to reflect these distinctions. |
HonahX
changed the title
How about we do review of #906 and #808 in parallel. If #906 merges first I will follow option 1. If #808 merges first, I can update #906 to also separate policy management APIs from IRC spec. Either way, this one will be closed finally. I leave it as "draft" now since we can have a reference point to see how things look when adding more APIs to the spec. |
HonahX
changed the title
Not ready for review, depends on #808
[Update] The draft PR to do the separation on current main branch spec is #906
#808 adds new APIs for policy management. These APIs are not IRC Apis (yet) so it is better to separate them into different yaml files according to Jack's proposal (https://lists.apache.org/thread/1fqocs00pno0xfr4ss2p69d6dv5h8qzf)
This PR is a PoC of one possible way to do the separation.