[SSSD] [PATCH v3] CI: Allow disabling distro-(in)dependent tests

Jakub Hrozek jhrozek at redhat.com
Thu Sep 18 16:16:04 UTC 2014


On Thu, Sep 18, 2014 at 03:55:47PM +0200, Lukas Slebodnik wrote:
> >From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
> >Date: Tue, 2 Sep 2014 17:45:22 +0300
> >Subject: [PATCH 1/1] CI: Allow disabling distro-(in)dependent tests
> >
> >Add support for --no-common and --no-distro options to contrib/ci/run.
> >
> >These options disable running distribution-independent and
> >distribution-dependent tests respectively, allowing running common
> >distro-independent tests separately.
> >
> >The mock builds and Clang analyzer run are considered
> >distro-independent. The test building, running them under Valgrind,
> >distcheck target and collecting and verifying coverage data are
> >considered distro-dependent.
> >
> >This allows CI service to run them on separate dedicated hosts, instead
> >of running on all hosts, speeding up overall execution.
> >---
> I really tried to avoid review this patches. I was expecting Michal will come
> to the meeting and explain his concerns, but someone had to ACK this patch.
> 
> This is the first time I read the patch. I'm so Sorry I have to reject it.
> I know that there was an agreement on meeting that we will push the patch.
> But on the meeting, there wasn't anyone who read/test the patch.
> 
> 1) I disagree that mock is distro independent patch.
>    The purpose of mock is to build sssd in clean environment. I gave you two
>    options: to use mock or to prepare clean Vm for each test.
>    Currently we do not buid sssd in clean enviroment for rhel7,
>    fedora-rawhdide and debian. The debian cannot be solved easily, but other
>    two options can be solved with mock. I mentioned it few times that we have
>    hardcoded mock builds for rhel6 and fedora-20. It means than on rhel7 we run
>    mockbuild for rhel6 and fedora-20.
>    Why? "The purpose of mock is to build sssd in clean environment", therefore
>    on rhel7 we should build with mock just for rhel7.

This sounds a fair point, but -- Nick mentioned inclusion of this patch is
blocking his rollout. If that's the case, I don't see a reason to hold
nack the patch over this issue. Let's file a ticket and move on to
something more pressing for us.

> 
> 2) clang analysis is ignored anyway. It will be easier to remove it, if we
>    want to speed up building.

Sounds like another ticket, not something that needs to be done now.

> 
> 3) I don't like solution how you solved the problem.
>    You added 6 if-else statements(some of them even nested) for 2 new command
>    line options. Sorry, It does not scale and script is much more complicated.

And? This is a test script. In the deamon we should totally build
readable, debuggable code because chances are you will be reading the
code 5 minutes before a deadline while an unhappy user is waiting for a
fix. Not here.

About scalability -- do we care too much? The way I see the future
development of this script, we'll likely add a couple of features like
the ones you listed here, but the tests themselves (like: Check if a
user from AD can log in) will be developed in something else, maybe a
variant of the framework IPA uses.

> 
> You wanted to speed up process.
> So, here is my proposal:
>     * run mock build just one. (the same as testing platform)

After a quick thought I agree, let's file a ticket.

>     * enable tmpfs mock plugin on all platforms.

Sounds like a good idea, let's file a ticket.

>     * remove clang analysis, because it is ignored anyway.

I don't know the process into too much detail, but sounds like we should
file a ticket.

Overall, I don't see a reason to hold off this patch if it's blocking
Nikolai. Again, this is a test. If the pach /improves/ the situation,
let's push it and carry on. It doesn't have to be perfect.

You know, I wish we reviewed the critical AD provider patches into this
much detail..



More information about the sssd-devel mailing list