I would like to have this header info same as other files.

Same here.

Same here.

Same here.

Instead of json__(), I think you can use:

from flask_restful import marshal
from waiverdb.fields import waiver_fields
marshal(row, waiver_fields)

You can use the existing helper function for creating a new waiver.
from .utils import create_waiver
create_waiver(result_id=1....)

I am new to fedmsg, but I guess we should have a message plugin similar to ResultsDB.

https://pagure.io/taskotron/resultsdb/blob/develop/f/resultsdb/messaging.py

Will that save me much of anything though? I still have to provide all the values and in this case I don't want to just flush the change, I need to commit it to trigger the event.

Okay, I addressed all the inline comments. Out of curiosity, why isn't the full noticed >suggested in the license used?
As Dan suggested before, it would make it easier to maintain as we are using a reduced GPLv2+ header comment.

I am new to fedmsg, but I guess we should have a message plugin similar to ResultsDB.

We certainly could set up an entry point and create a plugin architecture to register a set of hooks, but is that worth the complication? Do we anticipate supporting dozens of messaging formats that should not just be included in the waiverdb codebase? I'm not opposed to the idea, but it seems like it's a little more than we need.

Not a big deal, but it would be good to be consistent with other tests by using the create_waiver helper method.

from .utils import create_waiver
waiver = create_waiver(session, result_id=1...)

Generally +1

If you really want me to change it I'm happy to do so, but my reason for using the ORM directly is that it's clearer what's happening within the test. With a wrapper function, I (as a reader of the test later when it starts failing) have stop reading this function and jump to another function. That can be helpful for large tasks, but for just instantiating an ORM object, I think it's better to not add the complexity of a function call.

All that said though, it isn't a big deal so if you want the wrapper used, I'll use it.

Fair enough.

Is this unblocked for merge? I think there are no standing concerns. :+1:?

Doh, I think there are merge conflicts here now after I merged #17.

Merge conflicts resolved!

Sorry, I think I've introduced some merge conflicts here after I merged #19.

Resolved again :smile:

Merging before we introduce another conflict!! :bullettrain_side:

I hate these pointless copyright declarations would rather we not have them... but anyway no big deal.

Just wondering why fedmsg is optional? Could we just make it a requirement? Also no big deal.

I'm not a lawyer so I just do what they tell me to do. I'm pretty sure claiming copyright is very important, though.

Based on how the issue was written, I assumed both messaging "plugins" should be optional. It doesn't add much complexity, so I thought it would be nice.

It's not, SFLC has a good analysis here: https://www.softwarefreedom.org/resources/2012/ManagingCopyrightInformation.html

What matters it that we track authorship of all contributions, which is what git gives us. These inline copyright notices just inevitably bit rot (for example as soon as we get a contribution on this file in the year 2018 the notice is a lie, or if we get a contribution from someone that is not Red Hat) and serve no actual purpose which is why I prefer not to include them.