URL:
https://github.com/SSSD/sssd/pull/5693
Title: #5693: Basics of 'subid ranges' support for IPA provider
pbrezina commented:
"""
Hi @pbrezina,
thank you for the review.
> * Please, provide some description to the first commit message.
Do you think it makes sense to keep 2 commits? I was going to squash them after review.
It's up to you, just make sure it has sufficient description.
> * Why conditional build? Why is build disabled by default?
This is MVP of experimental feature.
Corresponding code in shadow-utils is merged upstream, but there is no upstream release
available yet. We brought functionality to Fedora via patches. But I doubt other
distributions will do the same. I.e. out of Fedora/RHEL, this is at the moment for
enthusiasts who are willing to play with a new feature and who is capable to rebuild
shadow-utils from sources.
Moreover, I anticipate significant changes might be required once we have initial
feedback.
So... at the very least it's conditional to allow distributions to avoid installing
plugin that they can't use yet.
Ack. This is something to document in the commit message.
Once shadow-utils upstream release is done and widely used, Podman
patches released
([
containers/storage#882](https://github.com/containers/storage/pull/882)), etc (so it is
more or less easy to use), we can change default and build code / install plugin in
sssd-ipa, IMO.
Ok.
> ```
> * We don't use two blank lines between function definitions.
> ```
And this (monolithic text) makes me sad :) I didn't see such restrictions in
https://sssd.io/contrib/coding-style.html
:-) Even though something is not explicitly banned, it is not cool to differentiate from
the rest of the project.
> ```
> * Can the range be set also for trusted domain user? If yes, this implementation
doesn't seem to support it.
> ```
No, FreeIPA doesn't support it in MVP.
Code looks good to me. I'm going to test it.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/5693#issuecomment-887410280