Core: Remove credentials from LoadViewResponse#11432
Conversation
|
@nastra can you please add some pr description, as to why are we removing this ? |
|
This change was added 2 weeks ago to the Load Table View api, @amogh-jahagirdar was the reviewer. Since this wasn't in any released code I am good with removing it now since we can always add it back later. If @amogh-jahagirdar signs off I'm good to merge. |
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
As discussed during the catalog sync-up meeting today, this PR makes sense to me.
|
|
||
| Map<String, String> config(); | ||
|
|
||
| @Value.Default |
There was a problem hiding this comment.
As the credentials() method didn't exist in previous releases (it has been introduced recently), and as it can be problematic for "secure views", I think it makes sense to remove it before 1.7 release.
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Yeah adding credentials for views in this manner complicates the protocol if we want to later add secure views, I'm good with the removal here!
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM, as well, Thanks @nastra !
Thank you for the context @RussellSpitzer !
|
Thanks @nastra ! and Thanks @RussellSpitzer @jbonofre @danielcweeks @singhpk234 for reviewing, I'll just go ahead and merge |
6 participants
The original idea why credentials were added to
LoadViewResponsewas to pass back the credentials to the table that the view is referencing. However, we actually want to wait with this until we have the use case fully defined on how this will work and want to remove the functionality for now, as otherwise we'd have to maintain reading/writing credentials inLoadViewResponseforever.We'll we sharing a separate proposal on how to realize this shortly on the mailing list.