This should be in the multi-line form.

It's probably worth using Django's TextChoices enum for these going forward. That gives us stable constants to refer to while also giving us a choices attribute for fields.

This help text doen't give a lot of useful information. We should at least give an example.

This is missing a trailing period.

"Git"

"GitLab"

We normally have required before help_text (including in the field above), so we should make this match.

We never return a value from this function. in the success case.

"GitLab"

Missing localization.

Missing localization.

Instead of casting, we should pull out the variable and check its type, raising an exception if it's not a string. That'd avoid any surprises, since we're technically dealing with user input.

"GitLab"

Can you place this one alphabetically? All the rest seem fine.

Same note as above regarding type safety.

Can we make these keyword-only?

If we're inlining this, we should probably prefix with a _ so it's clear it's not a public function.
Ideally, we'd avoid inlining if we don't have to, though. Might be worth moving this out.

This is a little hard to read. Can we move name to the next line so it doesn't appear to be part of the lambda?

This is in the wrong import group.

Can you make these keyword-only?

These should all be , optional.

Can you make this multi-line so it's easier to read/maintain?
Also, we should pass save=False since we're setting things and then saving.

Typo: kwargrs -> kwargs

Nit: You could just decorate the above with @spy_for.
Same below.

Can you update these for keyword-only arguments?

You can just pass base_commit_id= in as a parameter and avoid the double-save.

Let's keep one per line.

Can we test the exact error?
Here and below.

Blank line between statements and comments.

We access self.review_request a lot, so let's initially set it locally, set self.review_request, and then access the local copy.

Can we just test the entire dictionary in one go? Avoid surprises later?

we normally do one per line.

Why 202? It's really unclear how this is used here and below. Can we document this?
I think more of the hook could really use some documentation to make the requirements and responses more clear, especially if 202 is correct.

The error isn't going to be very useful by itself. Can we add to it?

We're not consistent here and with the above or below logs. We should stick to a very consistent format for these.

While this should never fail, it'd be nice to do this check early before all the validation.

We shouldn't assert, as that doesn't survive optimized builds. Good for internal usage, but for type-checking user-controllable state, we need to do an explicit check and a raise.

No need for the blank line here.

I know we're using Pydantic to verify, but then why do we need to assert here? Don't we get the right type and the guarantee?

This is a bit vague. Can we say "Unknown build status for <something>"?

"URL"

Something we need to do better at is building of URLs involving user-supplied input. Going to be commenting about this on newer code more. Rather than plugging things into a path, we should be safely feeding values into URLs using quote(), so we don't risk a case where some stray / or ? risks a vulnerability.
There may be more places in this code that need to do this.

This has user-modifiable input, so we need to handle ValueErrors here.

id is a reserved function. build_id?

To be consistent with above logs, we should capitalize "Unknown".

You can just use GeneralComment.objects.create(...).
Also, one keyword arg per line.

So where I think this is going to bite us is that by having such a specific compatibility requirement here in rbintegrations, we're going to collide with Review Board or eventually some other third-party dependency, and with the wider rbintegrations compatibility range for Review Board, this will ultimately affect us elsewhere.
How wide a range can we safely apply here?
I feel like if we're going to support pydantic, it makes sense to have that dependency as part of the Review Board platform, and keep the dependency there. Of course, that doesn't help rbintegrations on older versions, but that's where we can work around that with a wider range.

Ship It!

Looks like this won't fit to our workflow anymore. Is it possible to derive from this in a separate custom extension and use this to rbintegrations?

This is called REVIEWBOARD_REVIEW_ID in jenkins integration. That should be unified.

never use "DONE_FAILURE" for gitlab-ci. There will  always be a possibility that something is temporaly broken. If you use "DONE_FAILURE" you cannot RETRY the pipeline from RB.
DONE_FAILURE is a nice concept from RB, but it doesn’t reflect the reality in production.

see above

see above

So you will post the "summary" just for the initial pipeline and ignores all of child-pipeline?
Well, most times the "initial" pipeline should be just one job to spawn a child-pipeline with all important jobs because the gitlab-ci.yaml needs to be patched first with the diff from review request.
So this summary is pretty useless and cannot be configured anymore.

I used detailed_status for this. Otherwise you cannot have "success with warnings" or something like this.

Why? The child pipeline is a more interesting part. So the direct link to that pipeline should have higher priority. Otherwise the developer need to click into parent-pipeline and then select child-pipeline. That is really annonying.

why not stage anymore?

this will use a lot of lines here... we have 51 jobs.

I changed it to GitlabError because "error 404" is not an authentication error.