kevinjqliu commented on code in PR #2175:
URL: https://github.com/apache/iceberg-python/pull/2175#discussion_r2191469681


##########
mkdocs/docs/configuration.md:
##########
@@ -339,25 +339,19 @@ catalog:
 
 | Key                 | Example                          | Description         
                                                                               |
 | ------------------- | -------------------------------- | 
--------------------------------------------------------------------------------------------------
 |
-| uri                 | <https://rest-catalog/ws>          | URI identifying 
the REST Server                                                                 
   |
+| uri                 | <https://rest-catalog/ws>        | URI identifying the 
REST Server                                                                    |
 | ugi                 | t-1234:secret                    | Hadoop UGI for Hive 
client.                                                                        |
-| credential          | t-1234:secret                    | Credential to use 
for OAuth2 credential flow when initializing the catalog                        
 |
-| token               | FEW23.DFSDF.FSDF                 | Bearer token value 
to use for `Authorization` header                                               
|
 | scope               | openid offline corpds:ds:profile | Desired scope of 
the requested security token (default : catalog)                                
  |
 | resource            | rest_catalog.iceberg.com         | URI for the target 
resource or service                                                             
|
 | audience            | rest_catalog                     | Logical name of 
target resource or service                                                      
   |
-| rest.sigv4-enabled  | true                             | Sign requests to 
the REST Server using AWS SigV4 protocol                                        
  |
-| rest.signing-region | us-east-1                        | The region to use 
when SigV4 signing a request                                                    
 |
-| rest.signing-name   | execute-api                      | The service signing 
name to use when SigV4 signing a request                                       |
-| oauth2-server-uri   | <https://auth-service/cc>          | Authentication 
URL to use for client credentials authentication (default: uri + 
'v1/oauth/tokens') |
-| snapshot-loading-mode | refs                             | The snapshots to 
return in the body of the metadata. Setting the value to `all` would return the 
full set of snapshots currently valid for the table. Setting the value to 
`refs` would load all snapshots referenced by branches or tags. |
-| warehouse          | myWarehouse                         | Warehouse 
location or identifier to request from the catalog service. May be used to 
determine server-side overrides, such as the warehouse location. |
+| snapshot-loading-mode | refs                           | The snapshots to 
return in the body of the metadata. Setting the value to `all` would return the 
full set of snapshots currently valid for the table. Setting the value to 
`refs` would load all snapshots referenced by branches or tags. |
+| warehouse           | myWarehouse                      | Warehouse location 
or identifier to request from the catalog service. May be used to determine 
server-side overrides, such as the warehouse location. |

Review Comment:
   nit: move this under `uri` since this is important :) 



##########
mkdocs/docs/configuration.md:
##########
@@ -368,11 +362,84 @@ catalog:
     header.content-type: application/vnd.api+json
 ```
 
-Specific headers defined by the RESTCatalog spec include:
 
-| Key                                  | Options                               
| Default              | Description                                            
                                                                                
                                                            |
-| ------------------------------------ | ------------------------------------- 
| -------------------- | 
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 |
-| `header.X-Iceberg-Access-Delegation` | `{vended-credentials,remote-signing}` 
| `vended-credentials` | Signal to the server that the client supports 
delegated access via a comma-separated list of access mechanisms. The server 
may choose to supply access via any or none of the requested mechanisms |
+#### Authentication Options
+- **SigV4**: For AWS services that require SigV4 signing.
+- **OAuth2**: For services that require OAuth2 authentication.

Review Comment:
   lets add these as 2 separate subsections. One for OAuth2, followed by SigV4. 
   OAuth2 has options are `oauth2-server-uri`, `token`, `credential`, along 
with the few from my comment on top
   
   SigV4 is an AWS specific protocol. these are all related to 
sigv4:`rest.sigv4-enabled`, `rest.signing-region`, `rest.signing-name`



##########
mkdocs/docs/configuration.md:
##########
@@ -339,25 +339,19 @@ catalog:
 
 | Key                 | Example                          | Description         
                                                                               |
 | ------------------- | -------------------------------- | 
--------------------------------------------------------------------------------------------------
 |
-| uri                 | <https://rest-catalog/ws>          | URI identifying 
the REST Server                                                                 
   |
+| uri                 | <https://rest-catalog/ws>        | URI identifying the 
REST Server                                                                    |
 | ugi                 | t-1234:secret                    | Hadoop UGI for Hive 
client.                                                                        |

Review Comment:
   `ugi` should actually not be in REST catalog. its a hive catalog option that 
was accidentally added here 
https://github.com/apache/iceberg-python/commit/5209874cd4685427520b67b64188f5e9919f2816#diff-497e037708cc64870c6ba9372f6064a69ca1e74d65d6195dcee5a44851e8b47dR151



##########
mkdocs/docs/configuration.md:
##########
@@ -339,25 +339,19 @@ catalog:
 
 | Key                 | Example                          | Description         
                                                                               |
 | ------------------- | -------------------------------- | 
--------------------------------------------------------------------------------------------------
 |
-| uri                 | <https://rest-catalog/ws>          | URI identifying 
the REST Server                                                                 
   |
+| uri                 | <https://rest-catalog/ws>        | URI identifying the 
REST Server                                                                    |
 | ugi                 | t-1234:secret                    | Hadoop UGI for Hive 
client.                                                                        |
-| credential          | t-1234:secret                    | Credential to use 
for OAuth2 credential flow when initializing the catalog                        
 |
-| token               | FEW23.DFSDF.FSDF                 | Bearer token value 
to use for `Authorization` header                                               
|
 | scope               | openid offline corpds:ds:profile | Desired scope of 
the requested security token (default : catalog)                                
  |
 | resource            | rest_catalog.iceberg.com         | URI for the target 
resource or service                                                             
|
 | audience            | rest_catalog                     | Logical name of 
target resource or service                                                      
   |

Review Comment:
   move these 3 to the OAuth section since these are oauth concepts 
   
https://github.com/apache/iceberg-python/blob/d48a08d2da96088524211f1ef9607e9a8c34ca46/pyiceberg/catalog/rest/__init__.py#L334-L335



-- 
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: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to