On Wed, 2015-02-18 at 05:33 -0500, Josef Skladanka wrote:
Adam,
the run_all code does not really make much sense to me, to be
honest. After some minor cleanup, the code looks like this:
171 def run_all(args, wiki=None):
172 """Do everything we can: test both Rawhide and Branched
nightlies
173 if they exist, and test current compose if it's different
from 174 either and it's new.
175 """
176 skip = None
177 (jobs, currev) = jobs_from_current(wiki)
178 print("Jobs from current validation event:
{0}".format(jobs)) 179
180 yesterday = datetime.datetime.utcnow() -
datetime.timedelta(days=1)
181 if currev and currev.compose ==
yesterday.strftime('%Y%m%d'): 182 skip = currev.milestone
183
184 if not skip.lower() == 'rawhide':
185 rawhide_ffrel = fedfind.release.get_release(
186 release='Rawhide', compose=yesterday)
187 rawjobs = jobs_from_fedfind(rawhide_ffrel)
188 print("Jobs from {0}: {1}".format(rawhide_ffrel.version,
rawjobs))
189 jobs.extend(rawjobs)
190
191 if not skip.lower() == 'branched':
192 branched_ffrel = fedfind.release.get_release(
193 release=currev.release, compose=yesterday)
194 branchjobs = jobs_from_fedfind(branched_ffrel)
195 print("Jobs from {0}:
{1}".format(branched_ffrel.version, branchjobs))
196 jobs.extend(branchjobs)
197
198 if jobs:
199 report_results(jobs)
200 sys.exit()
Which on lines:
177-178: Runs the OpenQA jobs for "current" event
180: Creates a yesterday's date (formerly done on three lines
in a weird way)
181-182: IIUIC checks whether the "current" compose is from
yesterday, and if so, then sets skip to either Rawhide or Branched
184&191: Fails terribly, when the if-clause on 181 was False
(because skip equals None in that case) => no job results will be
reported to wiki matrices
Whoops - I've somehow sent an older version of the patch. Which is odd
because I was using the same file to apply on qa-01. I fixed all of
the above in the next rev. Looks like I had copies of the patch lying
in two directories and I sent the wrong one :(
The 'yesterday' thing is intended to be conditional on a --yesterday
parameter (it's basically a hack for the fact that at the time I was
testing it, the 02-18 nightly compose hadn't finished, but I figured
I'd leave it in as an option because you might hit that scenario
again). Though you're right about doing it more efficiently, dunno how
I wound up with it the silly way it was.
In the latest version skip is initially set to ''. :)
Also, it is kind of non-clear at the first read, that what
it does is basically "when you should not skip rawhide, run jobs for
rawhide and do the same for branched".
I'd much rather see something like `if skip.lower() !=
'rawhide':` and with a proper comment
I'd already added the comments in the latest version. I tend to have
'not ==' rather than '!=' as a tic, not sure why, I don't mind
changing it at all.
I'm not really sure how the whole 181 if clause works, and why is
it
evidently always True, since you have not encountered the error.
I did encounter the error, don't worry ;)
The point of the clause is a scenario that was actually true
yesterday: the case where one of the day's nightly builds is also the
'current validation event'. Yesterday, 22 Branched 20150217 was both
the 'current validation event' and the day's nightly Branched compose.
It would be wasteful to run jobs for it twice. So what that clause
does is check if the 'compose' for the current event is the same date
as the date we're testing nightlies for (utcdate); if it is, we check
what 'milestone' the current event is for and skip running the nightly
jobs for that milestone, since they're probably (really, certainly)
the same thing.
When I was testing it would do this: find that the current event is
'22 Branched 20150217', run against that (or skip if it had already
done so in a previous test), figure out 'utcdate', realize that it was
20150217 - the same as the current event - read out 'Branched' as the
current event's milestone, set 'skip' to 'branched', and therefore not
schedule any 'branchjobs'.
It could print a message when it skips a nightly for this reason, but
you can actually already tell, because it won't print a "Jobs from:"
message for that nightly. Unless it's skipped it will always print
such a message, even when there are no jobs.
Also, you mention a --yesterday parameter, which I have not really
found in the code.
Yeah, later version :/
I pushed the slightly polished code to the repos, so make sure to
pull :)
Whoops, please don't push stuff it it seems broken, I trust your
review :) I'm attaching a patch on top of your last commit which
should clean things up.
I'd really love to see some more comments in you code, which is
using the wikitcms/relval internal attributes and so on (it gets
somewhat wild in places).
I often go back and add comments at the last step, that's why they're
missing from the version that got sent out. I'm not sure what you mean
by 'internal attributes'; all the bits of wikitcms that I've used in
this code should be considered public. Is there some convention I'm
missing there? I thought the convention was that _ and __ prefixes are
for private stuff. But anyhow, yeah, things like the version
attributes (release, milestone, compose, version) and event.ff_release
are certainly intended to be available for outside use. I'm explicitly
trying to make it so you can throw (release, milestone, compose)
triples around at fedfind and wikitcms and they'll handle it as
gracefully as possible. Especially, you should always be able to call
relval commands on a fedfind-derived release, milestone and compose
and it'll do the right thing (I hacked in handling of the Rawhide
nightly case via release number guessing in wikitcms
get_validation_event()).
Please have a look at the run_all() method and:
* make sure that it handles the possible exceptions (please really
do _not_ use empty except clause, it is the root of all evil)
I kinda skimped handling any possible exceptions from
fedfind.release.get_release() in run_all() because it should always be
calling it in a valid fashion and hence should never hit one;
get_release() doesn't raise exceptions just because the release
doesn't exist, or anything, it only raises exceptions when it's called
in an inherently nonsensical way. But we can add it quite easily.
* document the if-clause on #181 and what it means
Patch attached which more or less brings it to my intended version,
with !='s and get_release() exception handling added and your cleanups
preserved. sorry for the mixup! Consider the previous version an
example of my personal kinds of silly thinkos that get fixed up when I
first test the code :P
To answer your question from IRC: 'all' will run on a max of three
composes. So the way we have Coconut set up ATM it'll run on a max of
six images: both arches for all three composes.
During the phase where nightlies are getting nominated it should never
do six; on the days a new nightly gets nominated it'll run for it as
'current event' and skip it as 'nightly', on other days it'll skip
'current event' because it's already been done.
After Alpha TC1 it'd run for six images each day we release a new
TC/RC (and both Rawhide and Branched nightly composes succeed). Other
days it'd do a max of four.
Obviously if we add more arches or images to the test suites, this
goes up accordingly.
--
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net