URL: https://github.com/SSSD/sssd/pull/412 Author: mzidek-rh Title: #412: Gpo contributed patches Action: opened
PR body: """ Hi, I am moving the patches from this PR: https://pagure.io/SSSD/sssd/pull-request/3320
Here on GH. The issue was stalled for a long time, and I do not think it is good idea to ask for changes in the patches after 7 months. So I decided to immediately change all the problems I found and squshed them into the patches in this PR.
There were no big issues with the patches: - I rebased it on top of the current master - added missing include for toupper function - the debug message format was not the same as we use on other places (note that this change was difficult to squash for each patch, so not all patches are 'clean', because I squashed all the debug message issues to the GPO: Improve logging of GPO... patch, however the result after applying all patches is as expected) - I added some comments - there were some memory leaks (missing frees)
Notice that I am not the author of the patches, only reviewer. The patches as they are in this PR LGTM. I tested them to check if something is broken (manually.. sigh) and now I am waiting for the CI results.
There was one issue occuring when I tested these patches and I originally though the is caused by these patches, but now I know it is not. The issue is that I did not have GPOs distributed to all DCs and it looks like SSSD randomly picks one DC and contacts it (even though it was DC from subdomain) so it could not always find all the GPOs (it looked for them on wrong DCs, where they were not). Other then that I encountered no issues with the patches, so it is ACK from me (tentative).
However since I am not native speaker and the last patch is MAN page change, I would appreciate second pair of eyes for that patch. Second pair of eyes for the rest of the patches would also be appreciated :)
However if nobody raises any issues I will ACK it soon (given the tests will pass). """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/412/head:pr412 git checkout pr412
URL: https://github.com/SSSD/sssd/pull/412 Author: mzidek-rh Title: #412: Gpo contributed patches Action: edited
Changed field: body Original value: """ Hi, I am moving the patches from this PR: https://pagure.io/SSSD/sssd/pull-request/3320
Here on GH. The issue was stalled for a long time, and I do not think it is good idea to ask for changes in the patches after 7 months. So I decided to immediately change all the problems I found and squshed them into the patches in this PR.
There were no big issues with the patches: - I rebased it on top of the current master - added missing include for toupper function - the debug message format was not the same as we use on other places (note that this change was difficult to squash for each patch, so not all patches are 'clean', because I squashed all the debug message issues to the GPO: Improve logging of GPO... patch, however the result after applying all patches is as expected) - I added some comments - there were some memory leaks (missing frees)
Notice that I am not the author of the patches, only reviewer. The patches as they are in this PR LGTM. I tested them to check if something is broken (manually.. sigh) and now I am waiting for the CI results.
There was one issue occuring when I tested these patches and I originally though the is caused by these patches, but now I know it is not. The issue is that I did not have GPOs distributed to all DCs and it looks like SSSD randomly picks one DC and contacts it (even though it was DC from subdomain) so it could not always find all the GPOs (it looked for them on wrong DCs, where they were not). Other then that I encountered no issues with the patches, so it is ACK from me (tentative).
However since I am not native speaker and the last patch is MAN page change, I would appreciate second pair of eyes for that patch. Second pair of eyes for the rest of the patches would also be appreciated :)
However if nobody raises any issues I will ACK it soon (given the tests will pass). """
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
mzidek-rh commented: """ Btw. if someone encounters the issues with the bad DC being contacted, the second lookup always contacted the correct DC for me. I will check if we track this issue somewhere already. """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-337705171
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
fidencio commented: """ While I do trust your review, I'd like to know whether we have downstream tests covering the area touched by the patches and have the downstream tests (not only the integration one) passing before we push it.
I personally would prefer to not have it in this release, than finding out a day or two after the release that we broke something in the current code.
So, can you run the downstream tests? """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-337705281
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
mzidek-rh commented: """ @fidencio Running downstream tests would be good indeed. But I do not know how to do it. Do you know if we have something better then the step-by-step gdoc guide on how to run the downstream tests? I am also not sure which tests should be run (which jobs to clone). """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-337872561
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
fidencio commented: """ @mzidek-rh, that's a problem we have and I don't know how to help you here.
I'd suggest to approach our QE guys on how to do that and have it ran before having this series pushed.
IMO it's a little bit too risky to add a huge series like this one one day before the release without having those tests green.
@lslebodn. do have spare cycles to help us with this one? """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-337883493
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
jhrozek commented: """ @mzidek-rh You can ask Dan Lavu to help you run the tests (sidecontrol on IRC) """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-338187148
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
fidencio commented: """ @mzidek-rh, I know you're busy with a few other stuff, but would be possible to at least give an update on this ticket? It's already been 24 days without **any** update.
So, Have you talked to Dan Lavu? What was the result of the tests? Which tests did you run in the end? """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-343841869
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
mzidek-rh commented: """ @fidencio Dan did run the tests and I had results, but the tests were all failing due to SSSD not been able to start. IIRC it was due to wrong selinux policy installed in the testing VM. So the results were not very useful in the end.
I will build a new package with the latest downstream packages (I think the SElinux policy was fixed now downstream) and give it to Dan or someone who can run the tests in greentea. """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-343873657
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
rdratlos commented: """ @mzidek-rh Sorry for my late reply. Thank you very much for reviewing the proposed patches and updating them. I have tested them on my systems and they still work perfect (sssd-1.16.0).
When reviewing the changes I identified two minor issues (typo, wrong debug level reference in man page). What is the best way to fix this before you ACK them? """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-345534508
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
lslebodn commented: """ On (19/11/17 17:33), rdratlos wrote:
When reviewing the changes I identified two minor issues (typo, wrong debug level reference in man page). What is the best way to fix this before you ACK them?
The best is to update pull request. Feel free to do it in pagure. If you already started there.
I have no idea why it was moved to github.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-345620461
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
mkosek commented: """ I do not see a problem in doing this PR here on SSSD GitHub space, it is where all the other PRs are being run and what we use in our infra. I see that Pagure is also apparently unable to work with this PR efficiently, estimated time for opening that PR link is 20 minutes... """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-345630286
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
lslebodn commented: """ On (20/11/17 08:55), Martin Kosek wrote:
I do not see a problem in doing this PR here on SSSD GitHub space, it is where all the other PRs are being run and what we use in our infra. I see that Pagure is also apparently unable to work with this PR efficiently, estimated time for opening that PR link is 20 minutes...
But there is a huge problem. The author of patches is Thomas Reim (@rdratlos probably) and he cannot update his own patches in this PR because author of PR is @mzidek-rh.
So the only feasible way is to create a new PR but he already did that on pagure few months ago. So there is not any reason to create 3rd PR.
Thomas already updated a PR on pagure but there is a merge commit "Merge branch 'master' of ssh://pagure/SSSD/sssd" and therefore there are 241 changed files with really huge diff and therefore it takes very long to open page. I've already asked Thomas in private mail to do proper rebase.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-345643881
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
mzidek-rh commented: """ Hi, I think it is OK if this PR is closed and a new one is opened here on GH by @rdratlos . It is also OK if he updates the PR on Pagure. Whatever works better for @rdratlos . I think using these patches as base for the new PR or rebase in PR will be easier (compared to the old PR on pagure), because some issues were fixed in them and they were rebased on top of master not so long ago. """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-345669157
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
rdratlos commented: """ Following an e-mail exchange with Lukas I have moved the patches, that have been updated by Michal here, back to Pagure and fixed the minor issues. See: https://pagure.io/SSSD/sssd/pull-request/3320. So for me it would be OK to close the PR and follow up the topic on Pagure. """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-345871313
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
rdratlos commented: """ Following an e-mail exchange with Lukas I have fetched the patches, that have been updated by Michal here, back to Pagure and fixed the minor issues. See: https://pagure.io/SSSD/sssd/pull-request/3320. So for me it would be OK to close the PR and follow up the issue on Pagure. """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-345871313
URL: https://github.com/SSSD/sssd/pull/412 Title: #412: Gpo contributed patches
mzidek-rh commented: """ Ok, closing this PR, we can continue on Pagure. """
See the full comment at https://github.com/SSSD/sssd/pull/412#issuecomment-345970378
URL: https://github.com/SSSD/sssd/pull/412 Author: mzidek-rh Title: #412: Gpo contributed patches Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/412/head:pr412 git checkout pr412
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
lslebodn
-
mkosek
-
mzidek-rh
-
rdratlos