modularize openqa_trigger -> fedora_openqa_schedule (T541)
ClosedPublic

Authored by adamwill on Aug 28 2015, 9:42 PM.

Details

Summary

This converts openqa_trigger into a fedora_openqa_schedule
package which is properly modularized: there's a CLI module,
a schedule module, a report module, and for now conf_test_
suites is its own module (though I think it's kind of ugly
and we should turn it into a JSON file or something).

ISO file download location configuration is now done with
an optional config file, as with the splits it becomes a
mess to try and pass it through from the CLI args. This also
means custom ISO locations will be respected by other things
we write which use the 'schedule' module.

This includes a setup.py so the package and fedora-openqa-
schedule command can be installed systemwide. We could now
extend this to install stuff like the systemd services and
little scripts like run-nightly.sh.

Test Plan

Check that things work more or less as before. New CLI command
is 'fedora-openqa-schedule'; it has the 'current' and 'compose'
sub-commands, plus a new 'report' sub-command which works like calling
report-job-results.py directly used to. Check that installing systemwide
works properly. Check that ISO download location configuration works as
expected. Running './fedora-openqa-schedule' from within the git checkout
should also work.

Diff Detail

Repository
rOPENQA fedora_openqa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
adamwill retitled this revision from to modularize openqa_trigger -> fedora_openqa_schedule (T541).Aug 28 2015, 9:42 PM
adamwill updated this object.
adamwill edited the test plan for this revision. (Show Details)
adamwill added reviewers: jskladan, garretraziel.
jskladan requested changes to this revision.Aug 31 2015, 11:33 AM
jskladan added inline comments.
fedora-openqa-schedule.py
2–7

I don't really see the point of this file. IIUIC it is not used at all, since setup.py uses the fedora_openqa_schedule.cli:main() directly.

Please remove this file.

fedora_openqa_schedule/cli.py
48

The class here has no real value, and only adds a ton of self to the code. Unless there is some reason behind using the class here, which I missed, please revert back to the (in the end more readable) scheme of non-encapsulated methods.

fedora_openqa_schedule/config.py
32–34

This is as unreadable as it gets, please use multiple CONFIG.read() calls instead.

fedora_openqa_schedule/report.py
106–112

OK, I understand the meaning, but it really does stink of bad library bahaviour.
How about

if not wiki:
    wiki = Wiki()
    wiki.login()
if not wiki.logged_in:
    logger.error("could not log into wiki")
    raise LoginException

instead?

This revision now requires changes to proceed.Aug 31 2015, 11:33 AM
garretraziel added inline comments.Aug 31 2015, 11:33 AM
fedora_openqa_schedule/schedule.py
86

I'm not sure whether we are downloading from HTTP(s) every time or whether are we using FTP sometimes. If we are using only HTTP(s), I would argue to replace urlgrabber with urllib.urlretrieve - it looks like urlgrabber's development is stagnating, urllib is standard library and only functionality urlgrabber gives us is that it can seamlessly use FTP also, but we have to use hacks like this.

garretraziel added inline comments.Aug 31 2015, 11:41 AM
fedora_openqa_schedule/schedule.py
38

I think that it would be better to import it as

from fedora_openqa_schedule.config import CONFIG

because there will hardly be any conflicts with local variables and you will not have to write config.CONFIG every time.

45–48

This solution is OKish for this one variable, but if (when) we add more values to be set by config file, this approach will require us to have all the defaults hardcoded and that is not nice. Wouldn't it be possible to set all the defaults in more sane way.

OTOH, if this is the only variable we are going to set by configfile ever, I guess that this makes sense.

garretraziel added inline comments.Aug 31 2015, 11:55 AM
fedora_openqa_schedule/cli.py
56–105

Move this functionality into schledule.py - create jobs_from_compose() (as there already is jobs_from_current()) and rewrite this function to be similar to current() function - only error handling, reporting and logging.

Imagine this scenario - if anyone will want to use fedora_openqa_schedule as a library, he will be able to use jobs_from_current() freely, but to get this behaviour (testing on specified compose), he will be required to duplicate much of this code - searching with fedfind, etc. I think that ideal state is that user of our library doesn't know about thirdparty libraries we are using (e. g. fedfind).

adamwill added inline comments.Aug 31 2015, 3:54 PM
fedora-openqa-schedule.py
2–7

It exists entirely so you can run the CLI directly from the git checkout. This was something kparal (IIRC) specifically *requested* that I add to relval a while back. It's a commonly-used pattern if you look around.

fedora_openqa_schedule/cli.py
48

eh, nothing in particular, it's either that or a ton of passing around args and client and wiki. I don't mind either way.

56–105

We can, but my idea was to make the module conversion one thing, and then major changes on top of that a different thing. I only made the changes necessary to keep things working as they currently do.

fedora_openqa_schedule/report.py
106–112

If by 'bad library behaviour' you mean the library can be not logged in, I disagree. The point is that you can do lots of useful things without being logged in - and in fact most of openqa-trigger does its stuff *without* being logged in. You only need to be logged in in order to report results. Your version would break in any case it's passed a 'wiki' instance that isn't already logged in (in fact I had it this way early, then spotted the hole and changed it). That's actually what *this* code currently does (CLI instantiates the wiki but doesn't log it in - it doesn't *need* to be logged in to do stuff like 'look up the current event', and logging in takes time, so we should only do it when we actually need to).

The auth store and 'default' default login behaviour is something I added in wikitcms (it's not in mwclient) so I could extend mwclient further and have it automatically try to login() if you try a save() operation without being logged in, but I'm not really sure if that's a great design in itself.

fedora_openqa_schedule/schedule.py
45–48

I went back and forth on it. I kind of hate configparser's method for setting defaults - amazingly, unless I'm missing something, you can't say something like 'the default for setting X in section Y is Z'. You can only say 'the default for setting X is Z' and it gets applied any time a setting called 'X' appears in any section. Which is completely bizarre and I can't find any good reason for it. So I kind of trolled around a bunch of different approaches to doing configuration and in the end fell back on 'screw it, stick with this thing that works for now'. If we do need to extend the config file we can use the stupid configparser mechanism or look at something else. I don't mind if you want to do it now, though.

86

Yeah, but that's completely outside the scope of this change, so I'd rather do it separately. I agree that since it's taken this long to get urlgrabber fixed in openSUSE we might want to look at something else, though. urllib only does HTTPS properly in recent Pythons, I think, but we probably don't care much about ones older than 2.7?

adamwill added inline comments.Aug 31 2015, 10:19 PM
fedora_openqa_schedule/schedule.py
45–48

OK, I've found a better way to do it now, just set the default values in config.py before reading in the config files (which will override the defaults for any values contained in the files).

adamwill updated this revision to Diff 1429.Aug 31 2015, 10:32 PM

drop Cli class, improve config handling

This addresses some of jskladan's review comments. The Cli class
is gone, default for 'iso_path' is done in (hopefully) a better
way, and I did the 'from foo import CONFIG' thing - definitely
a good idea. Other things were not done, for reasons explained in
the ticket.

OK, I revised to address some of jskladan's comments. Of the others, I think replacing urllib and moving some logic from cli.compose() into a new schedule.jobs_from_compose() are fine ideas, but I'd prefer to do them as separate follow-up diffs; the wiki.login() thing is more questionable but I think it's probably too minor to block merging? Let me know what you think. thanks! The jobs_from_compose() thing isn't completely straightforward - if you do it the obvious way you wind up with this as a signature:

def jobs_from_compose(wiki, client, release='', milestone='', compose='', ifnotcurrent=False, wait=0)

which seems a bit unwieldy. So I think it could probably benefit from its own diff to discuss the options.

garretraziel accepted this revision.Sep 1 2015, 6:57 AM

Ok, so apart from urlgrabber usage (I think that we should create another ticket about reviewing it), this looks good.

jskladan accepted this revision.Sep 1 2015, 6:59 AM
jskladan added inline comments.
fedora_openqa_schedule/report.py
106–112

My bad, I missed that you can do stuff without being logged in. /me feels ashamed

Macro tusken:       I went full tusken           you never go full tusken

This revision is now accepted and ready to land.Sep 1 2015, 6:59 AM
This revision was automatically updated to reflect the committed changes.