URL: https://github.com/SSSD/sssd/pull/947
Title: #947: tests: fix race conditions in integration tests
spbnick commented:
"""
@pbrezina, Hi Pavel :)
I don't really remember, sorry, but looking at the code now it might be to
avoid racing with SSSD initialization, while making th
URL: https://github.com/SSSD/sssd/pull/346
Title: #346: intg: Disable add_remove tests
spbnick commented:
"""
Sure, go ahead and push it. My comment is just my preference. I can go on to
explaining the reasoning, but I wouldn't want to force it on you and delay your
work.
&q
URL: https://github.com/SSSD/sssd/pull/346
Title: #346: intg: Disable add_remove tests
spbnick commented:
"""
This is a good move, and looks fine to me, except I would prefer to have a
comment in front of each disabled test saying it is disabled and briefly
explaining
URL: https://github.com/SSSD/sssd/pull/341
Title: #341: CACHE_REQ_SR_OVERLAY: Fix coverity warning
spbnick commented:
"""
Yes, this is absolutely correct, thank you, Fabiano, ACK!
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Woo-hoo :D! Thanks a lot for all the work, Pavel, Lukas and Jakub :)!
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
CI result:
http://sssd-ci.duckdns.org/logs/commit/17/e2bb8c81901721fc35f7807f8c000aea950c27/7063/summary.html
"""
See the full comment at
https://github.com/S
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Rebased, and swapped `cache_req_process_result` and `cache_req_done`, as
requested.
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
I'm on it, thanks for review @pbrezina!
"""
See the full comment at
https://github.com/SSSD/sssd/pull/136#issuecomment-305148541
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
This update rebases on latest master and also changes "tlog-rec" to
"tlog-rec-session", as the latter is now supposed to be used as the login shell
in upstr
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
CI results: http://sssd-ci.duckdns.org/logs/job/69/46/summary.html
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Alright, here is another version of the patchset. Changes include:
* abort SSSD startup if session recording is enabled, but session recording
shell is not present, or not e
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Yes, that's what I meant. Only perhaps we can just use
`--with-session-recording-shell=/bin/false` and not have any scripts.
"""
See the full comment at
https:/
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Integration tests don't actually need tlog to verify that SSSD's job is done
properly. Also, tlog is not going to be officially present for a while yet on
most distros we test
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
There is one other problem: integration tests. If we make SSSD fail on startup
when it doesn't find tlog-rec, then we can't run SSSD as is in integration
tests. That is without req
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Yeah, perhaps not starting SSSD can help.
Still, there is the issue of tlog not being available for RHEL at all, at least
not from official channels.
Should SSSD have session recor
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Sure, a weak dependency sounds right.
I would not like disabling SSSD or session recording in case tlog is missing.
If it can be fixed by just installing tlog, then we shouldn't brea
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Sure, a weak dependency sounds right.
I would not like disabling SSSD or session recording in case tlog is missing.
If it can be fixed by just installing tlog, then we shoul
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Sure, a weak dependency sounds right.
I would not like disabling SSSD or session recording in case tlog is missing.
If it can be fixed by just installing tlog, then we shoul
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Ah, I see. I understand what you meant regarding enable-files-domain. I'll try
to explain what we're trying to do with tlog.
First of all, session recording should always be disa
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Ah, I see. I understand what you meant regarding enable-files-domain. I'll try
to explain what we're trying to do with tlog.
First of all, session recording should always be disa
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Hi Lukas,
I'm sorry I don't quite understand. I agree that SSSD would have a runtime
dependency on tlog to use this feature, i.e. tlog would need to be installed to
work. Ho
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Pavel, I tried to address your latest comments. I had a difficulty with the
last one, though, the comment regarding "CACHE_REQ: Pull sessionRecording attrs
from initgr".
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Alright, I'll work on the manpages as well.
Another thing: should we also make a configure option for enabling session
recording, so that we can have this enabled and have an
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
BTW, should I perhaps include an update to the sssd.conf man page?
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Hi Pavel, thank you for your review! I'll be addressing your comments soon, but
for now here is how to test this.
The patches add support for a new section in sssd.conf: `se
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Pavel, I tried to address all your comments, and also added the fix you made to
data provider initialization regarding overrides. I also improved the tests.
This is ready for
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Pavel, after your suggestion on IRC I replaced `sysdb_initgroups` with
`sysdb_initgroups_with_views` in the data provider, and also switched to using
`sss_view_ldb_msg_find
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Pavel, after your suggestion on IRC I replaced `sysdb_initgroups` with
`sysdb_initgroups_with_views` in the data provider, and also switched to using
`sss_view_ldb_msg_find
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Thank you. Yes, I think it answers my question. I'll do another
sysdb_initgroups in the postprocess function.
"""
See the full comment at
https://github.com/S
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Thanks, Pavel.
I'm still confused, though. At the moment we invoke `create_initgr_ctx` with
an `ldb_result` returned from `sysdb_initgroups`. If `sysdb_initgroups`
returned `ENOEN
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Thanks, Pavel!
Is it ensured that the primary group name will always be in sysdb at that point?
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Pavel, another question: aren't we skipping the request altogether in
dp_initgroups, instead of just the post-processing callback here:
```C
ret = sysdb_initgroups(sbus_re
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Pavel, another question: aren't we skipping the request altogether in
dp_initgroups, instead of just the post-processing callback here:
```C
ret = sysdb_initgroups(sbus_re
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
> Primary group is specified with gidNumber and suplementary groups are
> specified with memberUid/memberOf. Primary group may or may not be part of
> suplementary
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Pardon for the review request, Pavel. Trying to figure out how they work on
GitHub. Please ignore.
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Hi Pavel, thanks a lot for a thorough review and for your kind words!
I'll be fixing the code according to your comments and will answer your
question below.
> I would welcome a
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
A better CI result: http://sssd-ci.duckdns.org/logs/job/66/48/summary.html
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Alright, this one includes PAM exporting the original shell as well. One thing
that bothers me about the implementation is that now all responders are reading
the shell settin
URL: https://github.com/SSSD/sssd/pull/136
Author: spbnick
Title: #136: Tlog integration
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/136/head:pr136
git checkout pr136
From 22256f94283bce43698b903f6ccb93e58031784c
URL: https://github.com/SSSD/sssd/pull/136
Author: spbnick
Title: #136: Tlog integration
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/136/head:pr136
git checkout pr136
From 871c2b596de3ae5238750ba6bd4a008a40e98138
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration
spbnick commented:
"""
Alright. Aside from `dp_req_initgr_pp` not letting us report a failure, I think
this is close to what we need, and is ready for a proper review.
"""
See the full comme
URL: https://github.com/SSSD/sssd/pull/136
Author: spbnick
Title: #136: Tlog integration WIP
Action: edited
Changed field: title
Original value:
"""
Tlog integration WIP
"""
___
sssd-devel mailing list -- sss
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration WIP
spbnick commented:
"""
@sumit-bose, @pbrezina, could you please take a look? I don't need a detailed
review yet, just feedback on approach. Thanks!
"""
See the full comment at
https:/
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration WIP
spbnick commented:
"""
Alright, this version loads override_space, and only does initgr request if
there are groups to match. I think I'll maybe add checking for
SYSDB_INITGR_EXPIRE to avoid firing ini
URL: https://github.com/SSSD/sssd/pull/136
Author: spbnick
Title: #136: Tlog integration WIP
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/136/head:pr136
git checkout pr136
From
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration WIP
spbnick commented:
"""
One thing bothering me is that `dp_req_initgr_pp` doesn't seem well suited for
what we're doing. I.e. nothing cares if it fails. Can we do something about it?
Change the
URL: https://github.com/SSSD/sssd/pull/136
Title: #136: Tlog integration WIP
spbnick commented:
"""
I've pushed a draft rewrite using cache_req and data provider. The
functionality is very basic and is mostly a proof-of-concept with no intent to
be efficient.
For each
URL: https://github.com/SSSD/sssd/pull/136
Author: spbnick
Title: #136: Tlog integration WIP
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/136/head:pr136
git checkout pr136
From
URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups
spbnick commented:
"""
All my comments were addressed, ACK on my points. Thanks, @fidencio!
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups
spbnick commented:
"""
@fidencio Sorry, I was able to understand very little from the comment, so I
can't really suggest a better commit message.
"""
URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups
spbnick commented:
"""
I'll try to review this.
"""
See the full comment at
https://github.com/SSSD/sssd/p
URL: https://github.com/SSSD/sssd/pull/136
Author: spbnick
Title: #136: Tlog integration WIP
Action: opened
PR body:
"""
@lslebodn, @pbrezina, this is the work-in-progress tlog integration patchset
I'd like to work on with you.
This is not for merging as it is. We can go over
URL: https://github.com/SSSD/sssd/pull/89
Title: #89: nss: rewrite nss responder so it uses cache_req
spbnick commented:
"""
Another question is cache_req going to be used by PAM (if this question even
makes sense)?
"""
See the full comment at
https:
URL: https://github.com/SSSD/sssd/pull/89
Title: #89: nss: rewrite nss responder so it uses cache_req
spbnick commented:
"""
Not a review, but rather a question to @pbrezina: why do some "plugin"
functions receive both cache_req_data and cache_req, which also r
URL: https://github.com/SSSD/sssd/pull/89
Title: #89: nss: rewrite nss responder so it uses cache_req
spbnick commented:
"""
In general it is good to submit your patchset to CI with the tag "each", to
have each patch tested (instead of only the last one), and
URL: https://github.com/SSSD/sssd/pull/83
Title: #83: TESTS: Check new line at end of file
spbnick commented:
"""
Ah, I see. Then you can put your patterns into a variable and check against
them in the loop, similarly to the way it's done above in the script. You can
use
URL: https://github.com/SSSD/sssd/pull/83
Title: #83: TESTS: Check new line at end of file
spbnick commented:
"""
Ah, I see. Then you can put your patterns into a variable and check against
them in the loop, similarly to the way it's done above in the script. You can
use
URL: https://github.com/SSSD/sssd/pull/83
Title: #83: TESTS: Check new line at end of file
spbnick commented:
"""
@lslebodn I left one suggestion, if that's not what you needed, could you
please specify in which way it should be "better"?
"""
See the
URL: https://github.com/SSSD/sssd/pull/52
Title: #52: CI: Remove dlopen-test from valgrind blacklist
spbnick commented:
"""
Also, if we stumble into something we can't fix again, we can always mask the
test again.
"""
See the full comment at
https://github.com/
spbnick commented on a pull request
"""
Just a hint: if you avoid putting "PR" in front of the PR#16, then you'll get a
link to the actual pull request on the GitHub page, plus the target pull
request will have a link back. Like this: #16.
"""
See the
60 matches
Mail list logo
