flyrain commented on code in PR #10256:
URL: https://github.com/apache/iceberg/pull/10256#discussion_r1590181761
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -215,6 +215,12 @@ public void initialize(String name, Map<String, String>
unresolved) {
this.paths = ResourcePaths.forCatalogProperties(mergedProps);
String token = mergedProps.get(OAuth2Properties.TOKEN);
+ // re-resolve these variables in case they were overridden by the config
endpoint
+ credential = mergedProps.get(OAuth2Properties.CREDENTIAL);
+ scope = mergedProps.getOrDefault(OAuth2Properties.SCOPE,
OAuth2Properties.CATALOG_SCOPE);
+ oauth2ServerUri =
+ mergedProps.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI,
ResourcePaths.tokens());
Review Comment:
It's a good idea to get certain configs from server. Thanks for doing this.
Should we add these into config endpoint spec if we are doing this? Without
something in the spec, we are relying on the convention. There is a minor
concern that the config response from the server side may happen to have the
same name with different meaning. For example, a property named `scope` in the
config response may mean different things. cc @danielcweeks
--
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]
