lmccay commented on PR #1043: URL: https://github.com/apache/knox/pull/1043#issuecomment-2912866647
> > @moresandeep - why add a new provider for this and not just add it to common so that it is available everywhere that extends that? > > Good point, I thought about that but decided against it in favor of code separation. Since all of the implementations use Common any bug might affect all the providers. Not all providers need this the ones that need this feature are virtual groups (already included in default) and hadoop-group-lookup so i decided to create a new provider that supports these. > > Putting this in common will be easy but this is not really common feature that can be used by all the other providers. Do you anticipate this to be used by others? i can move this common in that case. Okay, I am not sure that I like where it is but may need more coffee. It seems like the extension of impersonation that you are adding is decoupled from the default impersonation we have in common. That feels wrong but maybe there is a reason. It also appears to extend the HadoopGroupsProvider which I can understand as a possible dependency, meaning you would otherwise need two providers but putting the extension in common would give you that as well. I'll also point out that the mode has my head spinning a bit and I need to spend more time understanding that and possible edge cases. @pzampino - can you provide another set of eyes and opinion? -- 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: dev-unsubscr...@knox.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
