Store PRs in the database to avoid special-casing them during evaluation #1270
Conversation
There was a problem hiding this comment.
Apart from the missing return statement, everything looks good to me 👍
Something I thought that is not related yet to this is we should decide if we want to process PRs that were a result of a remediation and if yes, is there a chance that leads to some unhandled looping situation.
| @@ -132,6 +135,9 @@ func (s *Server) HandleGitHubWebHook() http.HandlerFunc { | |||
| log.Printf("Skipped webhook event processing: %v", err) | |||
| w.WriteHeader(http.StatusOK) | |||
| return | |||
| } else if errors.Is(err, ErrPrNotHandled) { | |||
| log.Printf("skipped PR processing") | |||
| w.WriteHeader(http.StatusOK) | |||
There was a problem hiding this comment.
I think we should also return here
Initially we stopped half-way when implementing PRs in the sense that the engine handled them, but we didn't store the PRs in the database. This worked fine until we added alert processing which needs the previous state of the entity being evaluated. Instead of adding even more special casing for PRs, it's cleaner to just store PRs in the database.
We'll have to upsert the PRs and delete them when closed.
Instead of checking the PR action before parsing, let's move the check to the webhook parser. To do that, let's also store the action in the protobuf message.
PRs not being opened, reopened or closed (which includes merged) are just ignored. Otherwise, opening or synchronizing a repo upserts the DB record, closing/merging removes the DB record. This PR would ease the way the engine is coded up by treating all entities pretty much the same. Fixes: #1241
I think we should process them, but I don't think we could loop. Opening a PR should only be done if no other PR with the same content exists yet. |
I was thinking if we can optimise the handling on our side and skip processing of open pr events that are created by us, but I guess the risks of missing something might be more than just making sure everything is verified and not be smart about it 😃 |
Initially we stopped half-way when implementing PRs in the sense that
the engine handled them, but we didn't store the PRs in the database.
This worked fine until we added alert processing which needs the
previous state of the entity being evaluated and the previous state is read
from the DB.
Instead of adding even more special casing for PRs, it's cleaner to just
store PRs in the database. That way, they can be processed exactly the same
as the other entities.
We only store opened PRs and remove closed PRs. There's no further processing
on the database entries.
Fixes: #1241