Would be nicer to use pytest tempfile fixtures, and then not delete it afterwards (let pytest's tempfile rotation take care of it instead). That way we can examine the sqlite db at the end of a test run if we need to.

What is waiverdb.db? Seems to be missing from this commit? Wouldn't we just import sqlalchemy stuff directly instead?

No need for this authorship info in the header, we have that from git as well.

I would like to avoid the copyright year above, for the same reason. Ideally we would use Beaker's reduced GPLv2+ header comment which is a bit easier to maintain than all this business.

A few files in this commit are missing the header as well.

I don't think we'll need this (nor the file logging below) since our deployment target will be at least RHEL7. We can just use systemd.journal.JournalHandler instead:

https://www.freedesktop.org/software/systemd/python-systemd/journal.html#systemd.journal.JournalHandler

(same as we did for rpm-test-trigger)

Copy-pasta :-)

Hmm I guess we are lacking a public mailing list right now... Maybe use qa-devel@lists.fedoraproject.org ?

You could also use an in-memory SQLite database for tests.

waiverdb.app.config['SQLALCHEMY_DATABASE_URI'] = "sqlite://"

I'd recommend using a decorator like this that will jsonify dicts, SQLAlchemy objects, and etc.
https://github.com/miguelgrinberg/api-pycon2015/blob/master/api/decorators.py#L8-L30

I find it's cleaner and you don't have to worry about returning JSON in your view. You'll have to change this though:
https://github.com/miguelgrinberg/api-pycon2015/blob/master/api/decorators.py#L22

Flask comes with "click" integrated now. This may be a better route.
http://flask.pocoo.org/docs/0.12/cli/

Your email address seems a tad strange here :)

Oh nice, yes we should definitely use that.

Matt (edit: I mean mjia) pointed out to me on IRC my mistake, waiverdb.db is an instance of SQLAlchemy which is a thing from Flask-SQLAlchemy, which I hadn't seen before.

What error/status code is returned when this fails?

Can you add a unit test for when an argument is not specified on this API as well?

This works, so it's up to you, but Flask-Migrate is pretty nice where it abstracts all this stuff for you.
https://flask-migrate.readthedocs.io/en/latest/

What was the reasoning behind a 36 character string for this? Seems arbitrary and small.

Should the timestamp be customizable?

You should probably convert this to ISO time.

You may want to consider putting this in config.py

That's a good tip, flask-migrate seems like a nice choice.

I wonder if we could just take out all the migration support from this first commit entirely, to keep it smaller? Maybe just have a single command that calls .create_all() for populating the schema. Then we can hook up flask-migrate in a follow-up commit later on.

That's a good point. We really don't need migration support until it's deployed in staging.

Non-blocking comment here -> I'd prefer if we just logged everything to stdout.

Then apache (or whatever server we choose) can redirect that to the appropriate place.

It makes waiverdb simpler if it doesn't have to care about all the different ways it could log its stuff.

Whatever we use, make sure it is the same format as timestamps from resultsdb.

https://taskotron.fedoraproject.org/resultsdb_api/api/v2.0/results for example

Yeah I would tend to agree, but using the systemd JournalHandler has some nice advantages: you get multi-line log messages (in particular, tracebacks) as a single journal entry instead of one for each line, and you get some extra structured metadata attached to each message for free such as the log level (errors turn red in journalctl, for example) and line number and source file.

I think logging to stdout is more compatible with a container-based deployment, which is something to consider.

Hmm, I will remove the authorship info and the copyright year then.

Yeah, we could change it later.

@Dan, do you mean the tempdir fixture?

@mprahl , I prefer to not using one global db for all the tests. Basically I want to create a db for each module.

So I guess by default waiverdb will log everything to stdout. The admin can change it to use the systemd JournalHandler as needed. What do you guys think?

reqparse.parse_args() will return validation error (abort(400)) . I will add a test for it.

Thanks for your tip, I like it, :-)

Agreed.

That is sth I am not sure either. By looking at the products we have released, the longest one seems to be 'RHEL-6-ComputeNode-Supplementary'. Which type would you recommend?

It could be but we can add it in as needed in the future.

Right.

I can not get systemd installed in my virtualenv as I hit an error like this

systemd/_daemon.c:283:31: fatal error: systemd/sd-daemon.h: No such file or directory
#include
^
compilation terminated.
error: command 'gcc' failed with exit status 1

I notice that MBS is doing like this as well, so I guess it might be for the same reason.
'

We use 5001 as a default port for resultsdb, 5002 for resultsdb_frontend and 5003 for execdb. So if you want to make this easily executable in dev mode with the rest of the stack without any reconfiguration, you might consider picking a different number :)

Ohh did all the inline comments disappear because you pushed an amended commit? :-S

For the length it needs to match whatever PDC uses for its product versions, which appears to be 200:

https://github.com/product-definition-center/product-definition-center/blob/master/pdc/apps/release/models.py#L108

Means you are missing the systemd-devel package.

But really, you should just be using the Python bindings that match the systemd package on the system itself, instead of building a random version from PyPI.

This is why using virtualenvs and downloading+building random stuff off PyPI is a losing game. Eventually you need to use system libraries at which point it's just nicer if everything was built consistently against everything else = just use the RPM packages provided by the distro.

This is surely a syntax error...

I guess it would be okay for doing like this now to make virtualenvs happy.

hmm, I could use 5004 then,

For logging to the journal, you don't need a format with all this junk in it -- that stuff is already sent as part of the structured journal messages. So for the journal code path, just don't set any formatter at all.

For the stdout logs this format also seems a bit verbose and hard to read, with all the hyphens. Maybe just use the same one as resultsdb, or just something nice and compact like %(asctime)s %(levelname)-7s %(message)s ?

:thumbsup: Nice! This looks like a good start. If you want to take out the WIP from the commit message, I think we can merge this and then iterate from there.

Unfortunately, Pagure wiped out our inline comments after you rebased it.
To reiterate, you said: "@mprahl , I prefer to not using one global db for all the tests. Basically I want to create a db for each module."

In that case, you should supply the scope='module' parameter to this decorator, unless it defaults to that already.

Additionally, I would run these commands in the beginning of your function if you want a clean database for every module:

waiverdb.db.session.remove()
waiverdb.db.drop_all()
waiverdb.db.create_all()

With those changes, you could use an in-memory SQLite database for tests as mentioned previously.

This should be configured in the TestingConfig class in config.py.

Then somewhere in this file you need to run:

waiverdb.app.config.from_object('config.TestingConfig')

After that, all your imports of app in your tests would be from the tests module and not from waiverdb.

I guess ideally we would avoid using the global waiverdb.app at all in the test suite. It gets messy (hard to reason about) when you have code that munges global config at import time...

I wonder if there is a way we can instantiate a new App instance (and corresponding database) in the fixture, and just set up the test config on that, instead of munging the global config?

@mprahl, @dcallagh I have refactored the code with application factories [1]. This allows me to use a global app associated with a db('sqlite:////var/tmp/waiverdb_db_test.sqlite') in the tests. FYI, I got the idea from this blog [2].

[1] http://flask.pocoo.org/docs/0.12/patterns/appfactories/

[2] http://alexmic.net/flask-sqlalchemy-pytest/

@mprahl, @dcallagh I have refactored the code with application factories [1]. This allows me to >use a global app associated with a db('sqlite:////var/tmp/waiverdb_db_test.sqlite') in the tests.

I've changed my mind to use tmpdir_fatctory to generate a temp db file which I think it is nicer.

Can you rename this file to factory.py? It's common to have the Flask app object be a global variable to the module. So when you do from waiverdb import app, you'd expect the Flask app object and in this case it's app.py file. It'd just make reading the code easier.

There are still some further tweaks we can make here (including @mprahl 's latest suggestion) but I am going to hit the button on this one now regardless, so that we have a starting point to file more PRs against.