@gnaponie @lucarval Can you review?

Well of course the subject type koji_build is koji_build! :smirk:

Can we turn this into a meaningful property instead? For instance: has_guessable_product_version.

Can we just have create_subject_from_data raise the exception instead? Maybe we can have a custom exception, UnknownSubject which gets mapped to BadRequest automatically?

Sort by id?

Maybe add some kind of is_deprecated field?

IMO has_guessable_product_version is less clear.

What I want to communicate with the attribute name is that whether it's possible to get the product version from Koji/Brew for the build ID (which is the subject item).

Why are we checking the secondary_item_key first? Its name implies that it should only be used if there's no match for the "primary".

to_json ?

IMO has_guessable_product_version is less clear.
What I want to communicate with the attribute name is that whether it's possible to get the product version from Koji/Brew for the build ID (which is the subject item).

Yeah... I don't have a better suggestion for it, but whenever we add a conditional in the code that checks the subject type, it's a warning sign of a gap in the abstraction layer.

Overall, I like where this is going. My main concern is being able to ensure that existing behavior is not broken.

Renamed secondary_item_key to item_key.

to_json ?

Renamed to_item_dict to to_dict (the value is not JSON yet).

Ah, is_deprecated is a good idea - I will add this.

Updated.

BTW, I used ignore_missing_policy instead of is_deprecated in bodhi_update subject type.

@lucarval @gnaponie Can we merge this before I start adding a code for new subject types? This would help me a lot.

"Could not" or "Couldn't"

It is unclear to me: are we still allowing with this change arbitrary subject_type? Because we should.

I still think subject types shouldn't be fixed, but the user should be able to ask for whatever, without asking us to introduce it, putting overload in the process. We made an effort in the past to do that, and we are making a step back with this. see: FACTORY-3583
I think that if we want to revert this we should talk about it first.

It is unclear to me: are we still allowing with this change arbitrary subject_type? Because we should.

Yes, greenwave.subjects.subject_type.create_subject_type() creates GenericSubjectType object if no configuration matches.

I still think subject types shouldn't be fixed, but the user should be able to ask for whatever, without asking us to introduce it, putting overload in the process. We made an effort in the past to do that, and we are making a step back with this. see: FACTORY-3583
I think that if we want to revert this we should talk about it first.

I'm still not sure what are your concerns.

This patch allows customizing behavior for any subject type using a yaml file. With this, Fedora and internal Greenwave instance can be customized separately, adding config files for other subject types or removing some of the default ones.

There are no fixed subject types.

Use can still ask for any subject type. If there is a yaml file for the subject type, it's behavior will depend on the configuration in the file.

Handling subjects/subject_types in the new submodules cleans up the code since there shouldn't be any checks for subject_type == 'koji_build' and similar.

This makes it easier and less error-prone to add special behavior for subject types.

Is there any downside to this? Is there other solution you have in mind?

I'm still not sure what are your concerns.
This patch allows customizing behavior for any subject type using a yaml file. With this, Fedora and internal Greenwave instance can be customized separately, adding config files for other subject types or removing some of the default ones.
There are no fixed subject types.
Use can still ask for any subject type. If there is a yaml file for the subject type, it's behavior will depend on the configuration in the file.
Handling subjects/subject_types in the new submodules cleans up the code since there shouldn't be any checks for subject_type == 'koji_build' and similar.
This makes it easier and less error-prone to add special behavior for subject types.
Is there any downside to this? Is there other solution you have in mind?

Alright then, I guess I misunderstood the change. It seems fine this approach.

This looks good... I guess. Please try to make more than one commit next time :)
it's really big and it's hard to review (to me).

+1

+1 (let's make sure we thoroughly test this in stage.)

@lholecek can we merge this then?