Add EiffelSourceChangeApprovedEvent

[e-backmark-ericsson]

Description

When a source change has been reviewed enough (fulfilling the rules on number of positive reviews for that specific change) it should be possible to send an event notifying that. In GitLab that state is called 'approved'. The EiffelSourceChangeApprovedEvent should have a CHANGE link to SCC.

Motivation

To fulfill a need on more detailed event notifications in the SCM area

Exemplification

There are use cases where a pre-merge pipeline should not be triggered before the change has been reviewed enough, for example to spare execution resources.

Benefits

Increased expressibility in the protocol.

Possible Drawbacks

None

[e-pettersson-ericsson]

Sounds reasonable to me, without in-depth knowledge about the area. What data would such an event contain (if any)? The name itself is probably enough information about what has happened. And with the link back to the SCC it is possible to find the source change in question.

Should the SCS be able to link to this proposed SCA(?) event? And in that case, how?

[e-backmark-ericsson]

It could be wise to include an optional data parameter where you could state how it was approved, and maybe by whom (multiple possible?)
I don't see any reason to link SCS to SCA. Both SCS and SCA would link to SCC with CHANGE link. But I see that ActT could link to all three events (SCC, SCA, SCS).

[magnusbaeck]

A couple of thoughts:

• Probably related to Emil's most recent comment, a change could be approved according to different criteria; a "normal" code review, an IP review, an API review, a regulatory compliance review, and so on. Each of these could trigger different activities so it would make sense to express what kind of approval was given. Should we aim for this granularity? And should we attempt to model pre-merge test activities (Jenkins job voting on a Gerrit change, GitHub checks, ...)? What would be the difference between this event and a CLM event pointing to the SCC under review?
• It should be possible to express that an approval has been withdrawn.

[e-backmark-ericsson]

@magnusbaeck , good catch there on the CLM event alternative. Actually I believe you could very well use a CLM event to signal that a source change (SCC) is approved, with arbitrary name in such a CLM (up to the user to standardize naming conventions there I believe). It could also signal that the source change has passed a certain level of automated pre-merge testing (through a pipeline automatically kicked off based on the SCC event).
A CLM event could also (at least theoretically) be used to decrease the level of confidence (withdraw approval), even though I've never seen that used in practice. It would require the consumers to always look for the latest CLM event linked to an SCC and not just rely on any approval CLM sent.
So, are we saying that this issue is unnecessary? We should at least add this scenario to an example showing how CLM could be used to signal approval of SCCs.

[magnusbaeck]

It would seem that this event type is superfluous, yes. Are you suggesting that we add such a scenario to the CLM documentation in EiffelConfidenceLevelModifiedEvent.md or did you have something else in mind?

[t-persson]

I would add it here https://github.com/eiffel-community/eiffel/tree/master/usage-examples with a new usage example for source change approvals.

I also believe that we should be careful with the phrasing and not say that it's used to "decrease the level of confidence" but rather to "signal that the confidence/approval was not met" as to be consistent with the other documentation.

@e-backmark-ericsson From the summits there have been talks about never being "allowed" to use CLM to decrease confidence, but I cannot find it in the documentation anywhere. Is this still valid and should it also be added to documentation?

[e-backmark-ericsson]

Yes, I agree with @t-persson . The CLM documentation is probably good enough in this aspect, but we should have a usage example showing how this could be used.

About decreasing confidence levels. I believe it is a bit troublesome to be a consumer of events that could signal a decreased confidence level, or rejected approval, or 'withdrawing' a previously published successful test case verification for example. There is an option today to send an IV event declaring that an issue succeeded verification and later on sending another IV declaring that someone changed it's mind and the verification should now be considered failed instead. The same possibility would be for a CLM that first has been sent to 'approve' an SCC and later another CLM is sent to revert/reject that approval. As an event driven system is asynchronous, it could be so that a rejected verdict could have happened but is not (yet) seen by the consumer, which could result in bad release decisions and such. But this is probably a whole different discussion to be kept in another issue.

I realize now that your statement on rejected approvals, @magnusbaeck , might actually not be about reverting a previously accepted source change, but just about rejecting it in the first place. Right?

[e-backmark-ericsson]

There is also a need to declare that a source change is "submittable", meaning that it is both approved by review but also verified through automatic tests. This state could maybe also be represented by CLM events pointing at SCC.

[e-backmark-ericsson]

Concluded in maintainers meeting 2023-09-14:
This issue should be transferred to only be a documentation issue. We should document how to use CLM events to show that a source change i approved.