Hmm, what problem have you seen? I can't see it as I got an error when running this migration script.
File "/home/mjia/waiverdb/waiverdb/models/waivers.py", line 27, in
MagicJSON = types.JSON().with_variant(StringyJSON, 'sqlite')
AttributeError: 'module' object has no attribute 'JSON'

Can we use s/result_subject/subject and result_testcase/testcase? I think adding 'result_' prefix is superfluous here.

Instead of carrying this code, I suggest we should use SQLAlchemy-Utils which is well maintained and developed.
http://sqlalchemy-utils.readthedocs.io/en/latest/_modules/sqlalchemy_utils/types/json.html
It is also available in koji.

https://koji.fedoraproject.org/koji/packageinfo?packageID=19060

Since Greenwave or other apps would query the waivers with those two columns quite often, we should use index on them.

I do not think we need to log sqlalchemy in the prod. If people want to debug it, they can simply set SQLALCHEMY_ECHO = True.
http://flask-sqlalchemy.pocoo.org/2.3/config/

would fields.Raw work?
http://flask-restful.readthedocs.io/en/latest/api.html#fields.Raw

This needs to update to reflect that this is a list of results identified by subject and testcase.

We also need to consider one case that subject and testcase are all missing in the dict.

I think it should group by testcase as well.

We should use a real example here which include more than one result. The response should match this as well.

It would be nice to move this into a classmethod of Waiver. Something like:
@classmethod
def by_results(cls, results):
return cls.query.filter(or_(*[....]))

Then you call Waiver.by_results(results) here and above.

Relates to greenwave#111.

Why is the argument named result when it's expected to be a list? I would imagine this should be named results.

Optional: I'd prefer if the list was created outside of the query for readability reasons, but this is fine.

Suggestion: Is it worth checking to see if subject actually has a value and that subject and testcase are indeed strings?

A more specific exception to catch would be nice since this could mask errors elsewhere and it would suppress a traceback that'd normally show up in the logs.

Same as above. I believe you want to catch the NotFound exception.

"erros" => "errors"

You can just replace this with:
if data.get('results'):

Re logging sqlalchemy - I'd just point out that accessing production logs is often problematic for devs. It's even harder if you have to instruct someone with access to the machine to run semi-random commands to get something useful from the logs.

Same as above.

Same as above.

You should move the data and api_url definitions out of the if blocks since all the if blocks repeat these two lines.

I would move this whole if block to after the execution of the other if blocks because you are repeating this same code.

Remove these automatic comments

Ooo, good find. That might do it. (I contributed this Unchanged thing.. let's try Raw, and if it works, lose the Unchanged field).

Nice find!

Good call. That will need a change both here and in the migration script.

But you wouldn't want to log all the SQL query outputs in production as most of the time they are useless and annoying.

I had problems altering and dropping tables with SQLite: http://www.sqlite.org/lang_altertable.html --> "SQLite supports a limited subset of ALTER TABLE."

If I try to add a column with "nullable=False", but no default value I get an error (it is ok to create a table with a column like that if the table is new, but not to add a column like that). And even if in the model I specify "nullable=False", but I specify "nullable=True" in the script it will be "nullable=True" in the database (and I don't want that).

So I thought to create the table and then alter the column, but there's no complete support in SQLite for altering column. So I had to use this "op.batch_alter_table" from alembic. And it seems to work.

Does it make sense?

Thanks, I see.

I've posted a new commit. It should be fine now.
Thanks

Ah, it looks like you've addressed the comments by just adding a new commit on top of the existing ones... I think that makes it a bit harder to review the whole thing, and it leaves a bit of a mess in the git history.

It would be cleaner if you could rebase the series and fix the issues in each commit where they are introduced. So that it keeps a clean, logical sequence of changes, rather than changing something in one commit and changing it again in a subsequent commit.

Also, it looks like you might have already rebased at one point and accidentally squashed some commits together? At least, when I try to read the series as is, some things don't seem to make sense to me. For example, https://pagure.io/fork/gnaponie/waiverdb/c/d8f5e2644552a83c47b1bcb6d1a51807f30ce124 says that it is "Waive the absence of result and dummy auth for CLI" but the actual changes appear to be just some unrelated tidying up of the code.

Ah, it looks like you've addressed the comments by just adding a new commit on top of the existing ones... I think that makes it a bit harder to review the whole thing, and it leaves a bit of a mess in the git history.
It would be cleaner if you could rebase the series and fix the issues in each commit where they are introduced. So that it keeps a clean, logical sequence of changes, rather than changing something in one commit and changing it again in a subsequent commit.

I'll try to clean it a little...

Also, it looks like you might have already rebased at one point and accidentally squashed some commits together? At least, when I try to read the series as is, some things don't seem to make sense to me. For example, https://pagure.io/fork/gnaponie/waiverdb/c/d8f5e2644552a83c47b1bcb6d1a51807f30ce124 says that it is "Waive the absence of result and dummy auth for CLI" but the actual changes appear to be just some unrelated tidying up of the code.

Yeah, because I had a lots of commits... so in the end I've just rebased almost everything, but maybe the comments weren't so good

@sochotni helped me to reorganize the commits.
@dcallagh do you think it is fine now?

You do not need to have two ifs here as you can just do:
if not isinstance(d.get('subject', None), dict):

Same as above.

Maybe we should give a clear message like "testcase is missing in 'results' parameter"

Same as above, you can just remove this line.

Same as this one.

This breaks backward compatibility as right now we are supporting for waiving multiple results. On the other hand, you could use tuples http://click.pocoo.org/5/options/#tuples-as-multi-value-options

And then you could parse the data as something like this:
for s in subjects:
data['subject'] = dict(s)

Note: we may need to check the given subject can be converted to a dict.

So to create a waiver, a command would be:

waiverdb-cli -t dist.rpmlint -s item=python-requests-1.2.3-1.fc26 type=koji_build -p "fedora-26" -c "It's dead!"

So is it possible that people want to filter waivers by testcase only? As I understand, by given a result like {'testcase': 'dist.rpmdeplint'}, it seems this method would return all the results?

@gnaponie , since the current Jenkins set up only runs after the PR is merged, you could create a job like the one I'm using(I will send you a link via email as we are using an internal redhat jenkins) and manully run it against your local branch. So this will you tell you at the early stage about whether your patch would affect other things before merging.

@mjia maybe I didn't understand your proposal... but is this way, how can the user specify different testcases for different subjects? How can you know which testcase is related to the subject?

We can consider the position, but maybe is a little tricky for the user to use it...
Example:
waiverdb-cli -t dist.rpmlint -s subject1 -t testcase1 -s subject2 -t testcase2 -p "fedora-26" -c "It's dead!"
The subject1 is associated to the testcase1 and the subject2 is associated to the testcase2.
Does it make sense?

I thought it could be a possible use case.
I guess I talked about it with Ralph.
Do you think is it not a possible use case?

mmm I think at this point there was some confusion (in my head at least...). You can filter without a subject... So if the user makes a request without specifying a subject (for example), this:
not isinstance(d.get('subject', None), dict)

would be -->
d.get('subject', None) will be None
not isinstance(d.get('subject', None), dict) will be True

so you will have a Bad request... even if you can filter without a subject (or testcase, or both).
Because I think the error message (for the bad request) should say to the user that he used the wrong format... and not that the parameter is missing.

Does it make sense?

This API doesn't make sense to me... not sure if I am just behind the times. But I thought that we changed it so that a waiver is recorded for a subject and testcase, right?

So if this is for selecting out waivers, and optionally filtering them, then surely it should just be two parameters (both optional), 'subject' and 'testcase' which filter the waivers down the requested value if the parameter is present.

This thing where it has to be a JSON-encoded array of dicts with 'subject' and 'testcase' seems very complex and difficult for the callers to use.

Who are the callers anyway?

Good point. As far as I know, no caller is calling this right now.

Okay, I did not realize that we would accept multiple testcases as well. So I guess we could use http://click.pocoo.org/5/options/#tuples-as-multi-value-options. An example would be
@click.option('--result', '-r', type=(unicode, unicode), multiple=True)
...
Then the caller could use a command like this to create multiple waivers

waiverdb-cli -r subject1 testcase1 -r subject2 testcase2 -p "fedora-26" -c "It's dead!"

And we need to clearly document that each result is in the form of 'subject testcase'.

It seems like we are going to change the API so I guess this part wouldn't be needed anymore.

If we are going to change the API to optionally accept subject/testcase, this is possible.

But still... a subject could be a complex data (a dictionary), and with Ralph we assumed that the user will provide the dictionary in this way (as example):
-s item=python-requests-1.2.3-1.fc26 type=koji_build
(and we split on the space and then on the '=').

It is for sure possible to add a testcase as you said, but it will be a little tricky for the user I guess... BTW I'll talk about it with Dan when he will be here in Brno, we will try to find a solution for this and we will write it here.

That's okay as you can have a callback for validation to make sure the result is in the form of 'subject' and 'testcase'.
http://click.pocoo.org/5/options/#callbacks-for-validation

Okay, had a quick chat with Giulia about this. I now understand the whole thing a lot better (that's my bad for not keeping up with all the discussions that were going on).

We think that, for the GET /waivers/ endpoint, we can simplify that to just accept optionally one testcase, and optionally one subject as a JSON-encoded dict. That will keep things simple. There are no actual known users of that API either, it is really just provided for completeness.

The API which Greenwave will use is the POST /waivers/+by-subjects-and-test-cases. It takes the more complicated JSON structure as the body of the request, which seems fine.

And I also suggested for the CLI for submitting waivers, to keep things simple we should just accept a JSON-encoded dict for the subject, rather than inventing some new 'key=value key=value' type of syntax. I personally think that having a space-delimited format (with spaces inside a single shell word) or an argument that accepts multiple words is difficult for people to get right. Typing out JSON like -s '{"item": ...}' is not very user friendly but at least the syntax is quite unambiguous. And hopefully users will not need to worry about the CLI anyway, we will have Bodhi waiving instead.

I made a new commit: 68e2b2e

Rebased and merged. Thank you!

Build 68e2b2e778e962d4caceabd36432cd75ff46dea9 FAILED!
Rebase or make new commits to rebuild.

Build 68e2b2e778e962d4caceabd36432cd75ff46dea9 FAILED!
Rebase or make new commits to rebuild.