laskoviymishka commented on code in PR #1369:
URL: https://github.com/apache/iceberg-go/pull/1369#discussion_r3539710979
##########
catalog/rest/rest.go:
##########
@@ -552,6 +593,10 @@ func toProps(o *options) iceberg.Properties {
setIf(keyPrefix, o.prefix)
if o.authUri != nil {
+ // Advertise the endpoint under the portable oauth2-server-uri
key while
+ // also retaining the legacy rest.authorization-url alias, so
existing
+ // consumers that read the properties back keep working.
+ setIf(keyOAuth2ServerURI, o.authUri.String())
Review Comment:
This is the one design call I'd want to settle before merge. Emitting both
keys means `r.props` always carries the endpoint under two names, and that
duplicate then gets cloned into table config downstream — two sources of truth
that can drift.
I get the intent is backward-compat for Go consumers reading props back, but
since we canonicalize to `oauth2-server-uri` everywhere else, I'd lean toward
emitting only the portable key here and dropping the legacy one. If we do want
to keep both, can we at least note in the comment that it's a Go-consumer
concession, not something other clients read? wdyt?
--
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]
