I'd skip this check - we might want to trigger RHBA/RHEA too.

why not just update default value of server_url to None in init, and useconf.errata_tool_server_urlas the errata url when noserver_url` is provided?

Not sure I understand logic why we raise an error here? Can you expand on this

I'd suggest to have triggered_by instead, if it is triggered by a user, it will be username or anonymous.

create deosn't honor the manual param, is it intended?

And generally, I think we don't need to emphasize it is manual_trigger in naming.

This is used to track if an event is triggered manually. triggered_by could be another field to store who triggered this event. Two different purposes.

If this event is triggered manually by calling the API, an error will be raised that means this manual trigger rebuild is not allowed, then will response 403 to tell that is a bad request.

I understand what happens here. I just don't understand why it should happen? Plus - the "manual" will be set to True when we manually (re)trigger specific advisory - so it will always fail here? Or am I misreading this completely?

when there is a value set for tiggered_by, that means it's triggered by external user/scripts, that's why we don't need the manual_triggered.

Authentication is added and some comments are addressed.

I understand what happens here. I just don't understand why it should happen? Plus - the "manual" will be set to True when we manually (re)trigger specific advisory - so it will always fail here? Or am I misreading this completely?

@sochotni can you point out which line this comment is for? I push --force before reading your comment. All previous inline comments are not shown now.

when there is a value set for tiggered_by, that means it's triggered by external user/scripts, that's why we don't need the manual_triggered.

It works obviously. But, I don't like this implicit way.

Repeating after force push:
I understand what happens here. I just don't understand why it should happen? Plus - the "manual" will be set to True when we manually (re)trigger specific advisory - so it will always fail here? Or am I misreading this completely?

Raising this error from here is for the API call. When rebuild is not allowed for given errata ID, response to client should contain this information as a bad request. See also test test_bad_request_if_not_allow_build_from_advisory.

handle returns [] whenever rebuild is not allowed or all work is done. Meanwhile, returning [] means no further events to be handled for next round of message loop through fedmsg-hub, that is not useful for an API call.

On the other hand, raising error could probably be a temporary solution, as it would be nice to make an asynchronous call to handle from post view method, which could save much time for client to wait until all rebuild images are found. In this way, it should not make sense to raise such an error.

Patch is updated to remove any potential tailing / from ET server URL.

OK, so in that case my suggestion is to skip the self.allow_build altogether for the manually triggered rebuild. If someone requests the rebuild the rules don't apply - it can be request to rebuild RHEA even outside of immediate whitelist rules. That's the whole point of the API - to do something that Freshmaker wouldn't normally handle automatically.

Not quite sure what you mean here.

As sochotni pointed out, raising BuildNotAllowed inside handler.handle seems confusing. It might be cleaner to refactor the code to take self.allow_build out of the self.handle and then here you can just do:
if not handler.allow_build(...):
raise json_error(404, 'Bad Request', str(e))

BTW, HTTP 403 is Forbidden, not Bad Request.

This should be 404, not 403.

Same as above.

Same as above.

BTW, HTTP 403 is Forbidden, not Bad Request.

@mjia Good catch. This is really wrong. I was intended to return a bad request response, so 400 is correct. Thanks.

This reuses handle, calling self.allow_build needs to refactor all relative existing code of handling messages got from fedmsg-hub. ATM, I'm not sure if it's worth to do this.

TODO: check if an advisory can be found by errata_id. If no, return 404 response.

This reuses handle, calling self.allow_build needs to refactor all relative existing code of handling messages got from fedmsg-hub. ATM, I'm not sure if it's worth to do this.

After reading Stano's comment, I'll reconsider this implementation.

Thanks. PTO today. :)

OK, I originally thougth we still should keep things limited by whitelist even in case of manual rebuilds, but Stano's point looks valid.

@cqi: Can you skip the allow_build completely in case of manual rebuild?

I'm little worried we are running the rebuild in frontend here.

I'm thinking about a way to do do this better. I will try discussing that with Ralph and Mike later today to find out if there are possible/valid. We could send a message to UMB from frontend and receive it in backend. That message would contain the information about the event backend should rebuild and we would handle that as any other message there.

@jkaluza Using UMB is a good point. :)

@jkaluza sure. I'll fix this in a separate commit before whole PR is good for merge.

cqi had to take unplanned long-term PTO, so we have agreed I will take his changed and finish them as separate PR. I'm closing this one without merging and will open new one soon :).