How about the following so that user's don't have to change their config?

RESULT_OUTCOME = app.config.get('RESULT_OUTCOME', ('PASSED', 'INFO', 'FAILED', 'NEEDS_INSPECTION'))

Then you could remove the entry in config.py above.

Thanks @mprahl. Added that. :)

:thumbsup:

Wouldn't this cause issues with DBs where the list of values is internally implemented as a set, when "what appears to be configuration" gets changed, but really, also a DB migration is needed?

I don't have any huge issues with the concept, but there are some nitpicks. One is in the previous comment. The other being, that at the moment, the list of allowed values for the OUTCOME is static, and you can expect to have the same available with any resultsbd you encounter.
While I understand that the list is by all means not exhaustive, and I agree that a simpler way of introducing "other values" is a good thing (tm), I'd much rather see this adding new configurable values to the set of already existing ones, instead of plain replacing the default set.
I also think, that with the set of "allowed" values now being dynamic, it would be nice to have that list at least semi-discoverable - it is certainly not the best solution, but adding a line with something like "allowed_outcomes" to the response on the / route would IMO be nice.
Do you think this is reasonable, or do you believe that just plain-replacing the outcome-set is a better solution to the problem? Also, what is the problem? :D

Wouldn't this cause issues with DBs where the list of values is internally implemented as a set, when "what appears to be configuration" gets changed, but really, also a DB migration is needed?

Quite possibly. Where would you expect to hit this? postgres?

I'd much rather see this adding new configurable values to the set of already existing ones, instead of plain replacing the default set.

Super idea. Will do.

...you [could previously] expect to have the same available with any resultsbd you encounter.

Good point. Only adding to the list instead of replacing it will help with this, but not resolve it. :(

it would be nice to have that list at least semi-discoverable

Yeah, agreed. Will add. :)

Also, what is the problem? :D

Heh, I've got two. The first is, I have some stakeholders that call for a NOT_APPLICABLE outcome as a requirement. I suggested using INFO for this, but they want to use both for different purposes. The second is the problem that execdb solves in the taskotron world. People really want to be able to see "pending" or "scheduled" when a result is absent.. and for good reason!

Quite possibly. Where would you expect to hit this? postgres?

Yeah, Postgres is one that I know of (and we use it...) I'm not sure how to do this in a sane way :( I guess people will have to read the docs, but we all know how that goes :D
Another solution could be changing the enum type to string (I'd rather see that limited to a reasonable amount of characters, even though this is just a gut feeling and not really based on any kind of facts), and thus eliminating the need for the DB migration. Heck, this is probably the solution, the more I think about it.
I'm of course very open to more informed reasoning :)

Good point. Only adding to the list instead of replacing it will help with this, but not resolve it. :(

That is true, but IMO having a "standard set plus extras" is a bit better than "random different sets" (exaggerated but I think you know what I mean). Not ideal, but has less of the stink, and I can't think of a better solution anyway. There still could be an argument made for just not specifying the outcome set at all, but I feel that a reasonable default is better than free-for-all.

Yeah, agreed. Will add. :)
Awesome, thanks!

Heh, I've got two. The first is, I have some stakeholders that call for a NOT_APPLICABLE outcome as a requirement. I suggested using INFO for this, but they want to use both for different purposes. The second is the problem that execdb solves in the taskotron world. People really want to be able to see "pending" or "scheduled" when a result is absent.. and for good reason!

Yeah, I can see that. We wanted to clearly separate those two, and I like that cleanliness of responsibility-separation, but I can understand that sometimes the beauty must make small step back for practicality.
And the design goal for resultsdb was "do whatever, this is a glorified key-value store anyway", so even though I would not be willing to do this in Taskotron, I don't see why this particular piece of code should be an obstacle in somebody else's happiness :D

Thanks @jskladan!

I'll add the db migration to convert from an enum to a string in here too (along with some of the other changes). It may not show up for another day or so, though. Will ping when it's ready for review again.

OK - should be ready for review again.

One thing I was unsure about - do you want the @validates validator for the outcome on the ORM level? There's plenty of validation already in api_v1.py and api_v2.py.

This is IMO not really needed. As you said in the comment, the validation is already done on the controller front,.
If there is a reason for keeping it, that I'm not seeing, maybe rename it to validate_outcome :)

Awesome, could you also add something like:

    def test_create_result_custom_outcome(self):
        self.test_create_group()
        self.test_create_testcase()
        ref_result = copy.deepcopy(self.ref_result)
        ref_result['outcome'] = 'AMAZING'
        r, data = self.helper_create_result(outcome='AMAZING')
        assert r.status_code == 201
        assert data == ref_result

Thanks!

Apart of the nitpicks above, this looks good to me!

Done in 23628ca.

Heh, agreed. Done in 633b7c4.

Cool! I'll test it and merge tomorrow. Thanks for bearing with me! :)

:+1: :)

closed by https://pagure.io/taskotron/resultsdb/c/827eeee629d1b48009b9c148007504df7f01166e?branch=develop

Thanks Josef!