Add missing enum values for detailed Merge Request status

[Wirone]

Recently our webhook started to throw error 500 again, it's a little bit unfortunate that external enum, which is out of control of this package, can affect production systems because of newly added values... Maybe it should be considered to fallback to string value if enum can't be created? All in all, the value sent by Gitlab to the webhook endpoint has to be correct 😅. Maybe enums like detailed_merge_status_enum.rb should be parsed in a scheduled job and compared with package's enum, so it would fail early and could be fixed instantly?

[mikaelpopowicz]

Hi @Wirone

Thank you for the PR. For the moment we don't plan to add a fallback mechanism as we prefer to give an enum to the users of the package, if things change upstream, the webhooks are bound to break nevertheless.

Feel free to open a PR if it can provide a strong alternative

Regards

[Wirone]

if things change upstream, the webhooks are bound to break nevertheless.

Not really. Imagine you have a class that handles some callback and receives MergeRequestEvent. If this listener has logic that depends on detailed merge request status, it probably has some conditions that trigger logic if MR is in particular status. Now imagine Gitlab add new kind of detailed status:

• in current implementation enum can't be created and webhook processor throws an exception, causing error 500. Yes, we have those errors in Sentry, have Slack notifications, but we have to drop what we do, fix the enums, create a PR and wait for the merge and release (or use the fork and update dependency directly in the app). This is cumbersome and wastes our time.
• in alternative implementation, where values sent by Gitlab are mapped as-is, the event object is created and most probably new status is not covered by listener, so it does nothing. In pessimistic scenario it can enter some condition just because new value was not excluded there, but the same will happen after adding new value to the enum, because most probably you don't review your listeners every time dependency is updated.

Look at this:

<?php

// This package requires PHP 8.1, so `readonly` on property level is OK
final class DetailedMergeRequestStatus {
    public function __construct(public readonly string $value)
}

// Creation during payload deserialisation
$status = new DetailedMergeRequestStatus($gitlabPayload['object_attributes']['detailed_merge_status']);

// Usage in the listener
final readonly class ProcessMergeRequest
{
    public function handle(MergeRequestEvent $event): bool
    {
        $event->mergeRequest->detailed_merge_status->value; // Just like enum, but supports ANY value
    }
}

In general, values sent by Gitlab to the webhook endpoint are always valid. App which acts as a webhook receiver always gets proper data, it does not make sense to end up with fatal error only because the library that handles deserialisation does not know about new possible value in the external dictionary.

This is especially important because webhook listeners might not even use such field (represented by enum) and with current approach they does not receive the data at all, which causes data loss (incomplete processes, invalid state etc). We had to fix our database manually several times because of errors caused by these enums. It would be much better if external dictionaries were flexible, and would have supported any value sent by Gitlab.

[Rezrazi]

We would consider a PR to improve things 😄