On (11/03/16 17:20), Dan Lavu wrote:
WOAH, that's a lot of comments! ;) Thanks for taking the time, I
questions inline and I'll start working on improving the code.
On Fri, Mar 11, 2016 at 1:16 PM, Nikolai Kondrashov <
> 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
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
> 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
> This is Debian Stable and we don't support that - we support Debian
> 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
However you need to update CI dependencies.
diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh
index c9a8a63..ef5b89b 100644
@@ -44,6 +44,7 @@ if [[ "$DISTRO_BRANCH" == -redhat-* ]]; then
sed -e 's/@PACKAGE_VERSION@/0/g' \
@@ -114,6 +115,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then
>> diff --git a/src/tests/intg/ca.py b/src/tests/intg/ca.py
>> new file mode 100644
>> --- /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
@@ -1,6 +1,7 @@
dist_noinst_DATA = \
+ ca.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.
>> + 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
- open(os.path.join(self.cert_dir, socket.gethostname() + '.crt'),
- .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 $?