On (11/03/16 17:20), Dan Lavu wrote:
WOAH, that's a lot of comments! ;) Thanks for taking the time, I
have some
questions inline and I'll start working on improving the code.
On Fri, Mar 11, 2016 at 1:16 PM, Nikolai Kondrashov <
Nikolai.Kondrashov(a)redhat.com> wrote:
> Hi Dan,
>
> Thanks a lot for your work! Please see my comments below.
>
> On 03/11/2016 06:29 AM, Dan Lavu wrote:
>>
>> Updated patch is attached,
>>
>> There were a few more packages I had to install to get CI running for
>> Debian, should we had these to the makefile?
>>
>> root@sssd2:~# apt-get python-openssl
>>
>> dpkg
>> -ihttp://
ftp.us.debian.org/debian/pool/main/n/nss-wrapper/libnss-wrapper_1.1.2-1_a...
>> dpkg
>> -ihttp://
security.kali.org/pool/main/l/linux/linux-libc-dev_3.16.7-ckt20-1+deb8u4_...
linux-libc-dev was installed as a dependency of other packages in my case.
>
>
> I'm not sure if libnss-wrapper is in Debian Testing, but we certainly
> shouldn't be downloading packages from a URL, especially non-debian
> packages,
> but use the repos configured on the machine.
>
>> Below are test runs against Debian and Fedora
I understand that completely, but I do feel that we need to find a way that
executing bash contrib/ci/run to setup CI should run successfully without
any other additional work.
All necessary steps are described in contrib/ci/README.md
(at least should be)
I was able to run contrib/ci/run on minimal debian:testing
without any problem. I had to have installed packages "git, sudo, lsb-release"
and configured sudoers as it is described in contrib/ci/README.md
I'm not familiar with Debian but I was not able
to find these packages in the base repositories.
They were not in base because
nss-wrapper was added later to debian.
And it isn't simple to add new packages to debian:stable.
That's the reason why we want to support only debian testing.
It's much simpler to get new packages there.
Also do we have a
bashrc_sssd equivalent for Debian?
No but contrib/ci/run should do all stuff.
>
>
>> root@sssd2:~/sssd/x86_64# cat /etc/debian_version
>> 8.3
>
>
> This is Debian Stable and we don't support that - we support Debian
Testing.
> Ping me if you need help installing that.
I'm testing and using the cloud-init openstack images, which is easy to
spin up, but why do we only support the testing release?
I explain it few paragraps
above.
However you need to update CI dependencies.
diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh
index c9a8a63..ef5b89b 100644
--- a/contrib/ci/deps.sh
+++ b/contrib/ci/deps.sh
@@ -44,6 +44,7 @@ if [[ "$DISTRO_BRANCH" == -redhat-* ]]; then
python-ldap
rpm-build
uid_wrapper
+ pyOpenSSL
)
_DEPS_LIST_SPEC=`
sed -e 's/@PACKAGE_VERSION@/0/g' \
@@ -114,6 +115,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then
python-ldap
ldap-utils
slapd
+ python-openssl
)
DEPS_INTGCHECK_SATISFIED=true
fi
>
>> diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py
>> new file mode 100644
>> index
>>
0000000000000000000000000000000000000000..c7adb7a2f0c2ea7f342c9df1d6ad41dcb2fcc28e
>> --- /dev/null
>> +++ b/src/tests/intg/ca.py
You need to add this file to Makefile
otherwise make distcheck will fail in contrib/ci script :-)
diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
index 7394997..0af59b8 100644
--- a/src/tests/intg/Makefile.am
+++ b/src/tests/intg/Makefile.am
@@ -1,6 +1,7 @@
dist_noinst_DATA = \
config.py.m4 \
sssd_id.py \
+ ca.py \
ds.py \
ds_openldap.py \
ent.py \
>> @@ -0,0 +1,163 @@
>> +#
>> +# SSSD LOCAL domain tests
>> +#
>> +# Copyright (c) 2016 Red Hat, Inc.
>> +# Author: Dan Lavu<dan(a)redhat.com>
>> +#
>> +# This is free software; you can redistribute it and/or modify it
>> +# under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; version 2 only
>> +#
>> +# This program is distributed in the hope that it will be useful, but
>> +# WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> +# General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not,
see<http://www.gnu.org/licenses/>.
>> +#
>> +
>> +from OpenSSL import crypto
>> +from os.path import exists, join
>> +
>> +import socket
>> +import os
>> +import shutil
>> +import config
>> +
>> +
>> +class CA():
>
>
> We're not basing CA on anything so there is no real need for parens and
> they're better be removed to avoid confusion.
>
Actually, we should extend object to be more python3 compatible.
e.g.
class CA(object):
>> + ca.get_subject().C = self.country
>> + ca.get_subject().ST = self.state
>> + ca.get_subject().L = self.city
>> + ca.get_subject().O = self.organization
>> + ca.get_subject().OU = self.unit
>> + ca.get_subject().CN = self.subject
>> + ca.set_serial_number(self.index)
>> + ca.gmtime_adj_notBefore(0)
>> + ca.gmtime_adj_notAfter(10 * 365 * 24 * 60 * 60)
>> + ca.set_issuer(ca.get_subject())
>> + ca.set_pubkey(key)
>> + ca.sign(key, 'sha1')
>> +
>> + if not os.path.exists(self.csr_dir):
>> + os.makedirs(self.csr_dir)
>> + if not os.path.exists(self.key_dir):
>> + os.makedirs(self.key_dir)
>> + if not os.path.exists(self.cert_dir):
>> + os.makedirs(self.cert_dir)
>> +
>> + open(os.path.join(self.cert_dir, cacert), 'wt').write \
>> + (crypto.dump_certificate(crypto.FILETYPE_PEM, ca))
>> + open(os.path.join(self.key_dir, cakey), 'wt').write \
>> + (crypto.dump_privatekey(crypto.FILETYPE_PEM, key))
>
>
> Can we use "with" statement here, so that file is closed ASAP, instead of
> waiting for GC?
>
+1 for "with" and extra variable for path. It's much simpler to
read
the code.
@see
- open(os.path.join(self.cert_dir, socket.gethostname() + '.crt'),
'wt')\
- .write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))
+ crt_path = os.path.join(self.cert_dir, socket.gethostname() + '.crt')
+ with open(crt_path, "wt") as f:
+ f.write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert))
> Aside from everything else above, please don't forget to run
pylint on the
> code before submitting!
>
pylint has some false positives due to pytest but we do not have any
pep8 issues in integration tests.
sh$ pep8 src/tests/intg/
sh$ echo $?
0
LS