9:24 a.m.
URL: https://github.com/SSSD/sssd/pull/373
Author: lslebodn
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Action: opened
PR body:
"""
Such simple test takes just 10 seconds and covers following regressions.
https://pagure.io/SSSD/sssd/issue/3283
https://pagure.io/SSSD/sssd/issue/3284
https://pagure.io/SSSD/sssd/issue/3485
https://pagure.io/SSSD/sssd/issue/3491
"""
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/373/head:pr373
git checkout pr373
9:26 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
lslebodn commented:
"""
There are definitely issues in this PR but it is enough for sanity testing.
And PR will probably need to get rid of ldbadd usage in favour of python bindings.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-327500334
11:14 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
NACK! As said by @lslebodn "fixing pep8 warnings without enforcing this check in CI
is a little bit wasted effort." and
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-327535286
11:17 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
I'm adding the "Rejected" label as "PR should be reviewd as whole patch
set and not just some parts.".
Feel free to remove the label when the Nacked patch is removed from the PR.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-327536054
11:17 a.m.
New subject: [sssd PR#373][+Rejected] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: +Rejected
11:21 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
NACK! As said by @lslebodn "fixing pep8 warnings without enforcing this check in CI
is a little bit wasted effort.".
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-327535286
3:46 a.m.
New subject: [sssd PR#373][-Rejected] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: -Rejected
3:46 a.m.
New subject: [sssd PR#373][+Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: +Changes requested
3:47 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
mzidek-rh commented:
"""
Hi!
@fidencio please do not use the flag 'Rejected' when requesting changes to the
patch set. Use 'Changes requested' instead. I did not review this patch, but
according to the conversation, you either want to add automated pep8 test or alternatively
remove the pep8 warning fix.
If the above is true, then I would prefer to remove the pep8 fix and create an issue to
enhance our CI to include pep8 check, because adding the test to this PR would be IMO too
much for one PR.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-327733950
3:47 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
mzidek-rh commented:
"""
Hi!
@fidencio please do not use the flag 'Rejected' when requesting changes to a patch
set. Use 'Changes requested' instead. I did not review this patch set, but
according to the conversation, you either want to add automated pep8 test or alternatively
remove the pep8 warning fix.
If the above is true, then I would prefer to remove the pep8 fix and create an issue to
enhance our CI to include pep8 check, because adding the test to this PR would be IMO too
much for one PR.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-327733950
6:37 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
lslebodn commented:
"""
I didn't try to fix all pep8 warnings as in different PR.
pep8 warnings template for config.py need to be fixed for easier backporting to older
branches due useless reformatting done in 53a4219
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-328080567
6:37 a.m.
New subject: [sssd PR#373][-Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: -Changes requested
3:08 p.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
Hmm. Makes sense.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-328368349
2:16 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
@lslebodn, when running the tests locally I noticed a lot of fails due to a missing
replacement, this patch is enough to fix it:
```
diff --git a/src/tests/intg/ds_openldap.py b/src/tests/intg/ds_openldap.py
index ccdff4cc8..842ff9108 100644
--- a/src/tests/intg/ds_openldap.py
+++ b/src/tests/intg/ds_openldap.py
@@ -245,7 +245,7 @@ class DSOpenLDAP(DS):
b"STRUCTURAL MUST ( cn ) MAY ( member $ businessCategory $ "
b"seeAlso $ owner $ ou $ o $ description ) )"),
]
- ldap_conn = ldap.initialize(ldapi_url)
+ ldap_conn = ldap.initialize(self.ldapi_url)
ldap_conn.simple_bind_s(self.admin_rdn + ",cn=config", self.admin_pw)
ldap_conn.modify_s("cn={0}core,cn=schema,cn=config", modlist)
ldap_conn.unbind_s()
```
Please, squash it in your patches.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-328439041
2:16 a.m.
New subject: [sssd PR#373][+Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: +Changes requested
6:57 a.m.
New subject: [sssd PR#373][synchronized] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Author: lslebodn
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/373/head:pr373
git checkout pr373
6:59 a.m.
New subject: [sssd PR#373][-Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: -Changes requested
6:59 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
lslebodn commented:
"""
calling `ldb_add` was replace with python version so we needn't add new dependency.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-328830995
3:48 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
- **intg: prevent "TypeError: must be type, not classobj":** Althought I
understand it would be out of the scope of this patch, but it could be fixed (not in the
same patch, but in the same series) also in:
- src/config/SSSDConfig/ipachangeconf.py:class IPAChangeConf:
- src/sbus/sbus_codegen:class Base:
- src/sbus/sbus_codegen:class DBusXMLParser:
- **intg: Let python paths be configurable:** ACK
- **intg: Add sanity tests for pysss_nss_idmap:** ACK
- **intg: Prefer locally built python modules:** ACK
- **ds_openldap: Extract functionality to protected methods:** ACK
- **intg: Create FakeAD class based on openldap:** Here I sincerely don't have enough
knowledge to tell whether it's right or not, but I trust the developer who wrote the
code ... so ACK (or someone else feel free to double-check this patch)
- **intg: Add sanity tests for pysss_nss_idmap:** ACK
Overall the series look good and, personally, I'd take advantage of this first patch
intention and also fix the very same issue in the three other places where it happens (and
I'm adding "Changes Requested" due to that).
Thanks for the series,
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-330160646
3:48 a.m.
New subject: [sssd PR#373][+Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: +Changes requested
3:51 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
lslebodn commented:
"""
On (18/09/17 01:48), fidencio wrote:
- **intg: prevent "TypeError: must be type, not classobj":**
Althought I understand it would be out of the scope of this patch, but it could be fixed
(not in the same patch, but in the same series) also in:
- src/config/SSSDConfig/ipachangeconf.py:class IPAChangeConf:
- src/sbus/sbus_codegen:class Base:
- src/sbus/sbus_codegen:class DBusXMLParser:
Yes, it is out of scope of this
patch.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-330161363
3:51 a.m.
New subject: [sssd PR#373][-Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: -Changes requested
3:55 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
Although I strongly would prefer all or nothing, let's do it in the way you wish.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-330162286
3:56 a.m.
New subject: [sssd PR#373][+Accepted] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: +Accepted
4:10 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
I've opened https://pagure.io/SSSD/sssd/issue/3517 and marked it as easyfix.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-330165208
6:57 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
lslebodn commented:
"""
There was a comment: someone else feel free to double-check this patch
Temporary removing ACK label so anybody else can check as well.
Timeout for temporary removed ack is 4 days. Then will be automatically added back :-)
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-330198194
6:57 a.m.
New subject: [sssd PR#373][-Accepted] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: -Accepted
9 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
And seems that the tests failed in all machines in our internal CI:
http://vm-058-233.${abc}/logs/job/77/52/summary.html.
Adding back the "Changes Requested" label till it's fixed.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-330231155
9 a.m.
New subject: [sssd PR#373][+Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: +Changes requested
3:55 a.m.
New subject: [sssd PR#373][synchronized] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Author: lslebodn
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/373/head:pr373
git checkout pr373
5:23 a.m.
New subject: [sssd PR#373][-Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: -Changes requested
7:57 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
CI passed: http://vm-058-233${abc}./logs/job/77/96/summary.html
ACK from me!
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-330842976
5:30 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
@lslebodn: Would you mind rebasing the patch atop of git master? Seems that we have a
conflict in src/tests/intg/Makefile.am.
As anyone else has manifested their opinion about
https://github.com/SSSD/sssd/pull/373#issuecomment-330198194 ... I guess we're good to
go with these patches as soon as they're rebased atop of master.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-332156569
5:52 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
jhrozek commented:
"""
I read the diffs (note: I haven't done any testing at all) and I like the patches. So
ack+1 from me.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-332161331
5:10 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
Adding the "Changes Requested" label till the conflict is resolved.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-333493630
5:10 a.m.
New subject: [sssd PR#373][+Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: +Changes requested
9:10 a.m.
New subject: [sssd PR#373][synchronized] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Author: lslebodn
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/373/head:pr373
git checkout pr373
9:18 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
@lslebodn, thanks for updating the patch.
Just for the sake of our process I'm firing a CI build and I'll re-add the
"Accepted" label as soon as I get the results.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-336151588
3:39 a.m.
New subject: [sssd PR#373][-Changes requested] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: -Changes requested
5:03 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
fidencio commented:
"""
@lslebodn, I've fired a few (~10) builds in our CI and the only two failures we had
were not related at all with those patches.
I'm adding the "Accepted" label.
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-336409372
5:03 a.m.
New subject: [sssd PR#373][+Accepted] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: +Accepted
8:17 a.m.
New subject: [sssd PR#373][comment] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
lslebodn commented:
"""
master:
* da7a3c347dd630085839afa7ec245ee9d36f6ce2
* 36df33cd44774a5b5eab52ab222bcd3240b3ca5a
* ebbd9a2b551feffd2040f35d938f6800fba1b278
* d82741b1a8ada493ca74efa5d5c8b731412d035c
* 948c1a4d423dba289342844fb177bd5fa9828fc0
* a3bed9df5a47bfc84b82341f0f7e693e2b14a67a
* 035bed97b8da9a7f3a2d15bcf0e184bd973977a7
"""
See the full comment at https://github.com/SSSD/sssd/pull/373#issuecomment-336883506
8:17 a.m.
New subject: [sssd PR#373][closed] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Author: lslebodn
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Action: closed
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/373/head:pr373
git checkout pr373
8:17 a.m.
New subject: [sssd PR#373][+Pushed] intg: Add sanity tests for pysss_nss_idmap
URL: https://github.com/SSSD/sssd/pull/373
Title: #373: intg: Add sanity tests for pysss_nss_idmap
Label: +Pushed
2377
days inactive
2417
days old
sssd-devel@lists.fedorahosted.org
43 comments
4 participants
participants (4)
-
fidencio -
jhrozek
-
lslebodn
-
mzidek-rh