[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