Flake8 complain about this line: E265 block comment should start with '# ' -- you are missing a space after the #

Flake8 complain for: E231 missing whitespace after ',', you need to add a space before requester.

I'm not pretty clear about in which situation the requester, requester_metadata and requested_rebuilds can be missed. It would be better to explain that in commit message.

But if that happens when the event is not found in DB, how about updating Event.get_or_create_from_event and Event.get_or_create to have these data recorded?

I see, so this is caused by the message published doesn't include requester, right? Could you also fix this in AsyncBuildAPI.post?

I see, so this is caused by the message published doesn't include requester, right? Could you also fix this in AsyncBuildAPI.post?

Yes, and when consumer can't find event in db he creates new one with info from message, and if message doesn't have requester, requester_metadata or requested_rebuilds, missing fields will be empty in new event.

I will fix it for AsyncBuild too.

I'm not pretty clear about in which situation the requester, requester_metadata and requested_rebuilds can be missed. It would be better to explain that in commit message.
But if that happens when the event is not found in DB, how about updating Event.get_or_create_from_event and Event.get_or_create to have these data recorded?

get_or_create can't be used because it doesn't have access to requester information, because if doesn't get Event object. And get_or_create_from_event could do populating of this info, but we are using get_or_create instead in places where processing new events. And those two functions do almost the same, but still they where created for something so I would let them be where they are now .

get_or_create can't be used because it doesn't have access to requester information, because if doesn't get Event object. And get_or_create_from_event could do populating of this info, but we are using get_or_create instead in places where processing new events. And those two functions do almost the same, but still they where created for something so I would let them be where they are now .

You can add these valid arguments to create and get_or_create, just like what you did for requester, for example:

    def get_or_create(cls, session, message_id, search_key, event_type,
                      released=True, manual=False, dry_run=False,
                      requester=None, requested_rebuilds=None,
                      requester_metadata=None):
    ...
    def get_or_create_from_event(cls, session, event, released=True):
        requester = getattr(event, "requester", None)
        requested_rebuilds = getattr(event, "container_images", None)
        if requested_rebuilds and isinstance(requested_rebuilds, list):
            requested_rebuilds = " ".join(requested_rebuilds)
        requester_metadata = getattr(event, "requester_metadata_json", None)

        return cls.get_or_create(
            session, event.msg_id,
            event.search_key, event.__class__,
            released=released, manual=event.manual,
            dry_run=event.dry_run,
            requester=requester,
            requested_rebuilds=requested_rebuilds,
            requester_metadata=requester_metadata)

which is easier to maintain.

get_or_create can't be used because it doesn't have access to requester information, because if doesn't get Event object. And get_or_create_from_event could do populating of this info, but we are using get_or_create instead in places where processing new events. And those two functions do almost the same, but still they where created for something so I would let them be where they are now .

You can add these valid arguments to create and get_or_create, just like what you did for requester, for example:
def get_or_create(cls, session, message_id, search_key, event_type,
released=True, manual=False, dry_run=False,
requester=None, requested_rebuilds=None,
requester_metadata=None):
...
def get_or_create_from_event(cls, session, event, released=True):
requester = getattr(event, "requester", None)
requested_rebuilds = getattr(event, "container_images", None)
if requested_rebuilds and isinstance(requested_rebuilds, list):
requested_rebuilds = " ".join(requested_rebuilds)
requester_metadata = getattr(event, "requester_metadata_json", None)

    return cls.get_or_create(
        session, event.msg_id,
        event.search_key, event.__class__,
        released=released, manual=event.manual,
        dry_run=event.dry_run,
        requester=requester,
        requested_rebuilds=requested_rebuilds,
        requester_metadata=requester_metadata)

which is easier to maintain.

I did a little improvement to my previous code, so now there is a function in Event class to populate requester fields it they are not filled yet. I think it could be better to have less optional parameters in get_or_create function to have it simpler to use and understand.

I did a little improvement to my previous code, so now there is a function in Event class to populate requester fields it they are not filled yet. I think it could be better to have less optional parameters in get_or_create function to have it simpler to use and understand.

With updating the Event's create functions, there will be no necessary to check event type and then fill the fields, and I think it makes sense to have these arguments in Event's create functions since they are columns of Event. What do you think?

I did a little improvement to my previous code, so now there is a function in Event class to populate requester fields it they are not filled yet. I think it could be better to have less optional parameters in get_or_create function to have it simpler to use and understand.

With updating the Event's create functions, there will be no necessary to check event type and then fill the fields, and I think it makes sense to have these arguments in Event's create functions since they are columns of Event. What do you think?

Yes i think you are right. I will change the code.

@qwan But I got it only now. If I will change get_or_create and get_or_create_from_event methods as you said and delete type checking. Still I will have to write this piece of code:
...
requester = getattr(event, "requester", None)
requested_rebuilds = getattr(event, "container_images", None)
if requested_rebuilds and isinstance(requested_rebuilds, list):
requested_rebuilds = " ".join(requested_rebuilds)
requester_metadata = getattr(event, "requester_metadata_json", None)
get_or_create(...)
...
everywhere where get_or_create method is used. Because it requires strings arguments for metadata and rebuilds. And I can't use get_or_create_from_event because their usage is a little bit different(e.g. we can't say to get_or_create_from_event method to make dry_run=False, as we do with get_or_create, same for released and manual fields).

So I am not sure what solution is better for this.

everywhere where get_or_create method is used. Because it requires strings arguments for metadata and rebuilds. And I can't use get_or_create_from_event because their usage is a little bit different(e.g. we can't say to get_or_create_from_event method to make dry_run=False, as we do with get_or_create, same for released and manual fields).

I believe we can change them (get_or_create) to get_or_create_from_event (at least for the ones in this PR), and event does have event.dry_run, event.manual, and you can pass released to get_or_create_from_event as well.

everywhere where get_or_create method is used. Because it requires strings arguments for metadata and rebuilds. And I can't use get_or_create_from_event because their usage is a little bit different(e.g. we can't say to get_or_create_from_event method to make dry_run=False, as we do with get_or_create, same for released and manual fields).

I believe we can change them (get_or_create) to get_or_create_from_event (at least for the ones in this PR), and event does have event.dry_run, event.manual, and you can pass released to get_or_create_from_event as well.

ok, then I will replace usages of get_or_create to get_or_create_from_event used in PR.

events always have manual and dry_run (check BaseEvent), so you don't need this.

requester_metadata has already been converted to string in views (_create_rebuild_event_from_request).

requester_metadata has already been converted to string in views (_create_rebuild_event_from_request).

I was wrong, ignore this please

The "container_images" can be "[]" (which is not None) in request, so this can break. We can just use

if requested_rebuilds_list and isinstance(requested_rebuilds_list, list):
    requested_rebuilds = " ".join(requested_rebuilds_list)
else:
    requested_rebuilds = None

The "container_images" can be "[]" (which is not None) in request, so this can break. We can just use if requested_rebuilds_list and ....

Yes but if it's empty list, it must be converted to string too, so thats why I have there 'is not None'

The "container_images" can be "[]" (which is not None) in request, so this can break. We can just use if requested_rebuilds_list and ....

Yes but if it's empty list, it must be converted to string too, so thats why I have there 'is not None'

ah, I see, you have set it to None by default, before the checking.

:thumbsup:

Looks good to me too :+1:

@apaplaus feel free to merge this