Add Provider types `githubpullrequestcomment` and `gitlabmergerequestcomment`

[matheuscscp]

With flux-operator's ResourceSet it became possible to use Flux for deploying ephemeral preview environments for GitHub Pull Requests and GitLab Merge Requests.

Users want better feedback on the pull/merge requests about the status of the deployed resources:

controlplaneio-fluxcd/flux-operator#212

Proposal

We could introduce two new values for spec.type in the Provider API: githubpullrequestcomment and gitlabmergerequestcomment. These two providers would create/update comments on the "change request" an event is targeting. A particularly important requirement for these providers is not to spam the change request i.e. during the lifetime of a change request many events would potentially happen, and creating a new comment for every event would be too noisy .e.g it would be hard to find the latest status and the change request would be immensely polluted.

For supporting these providers I propose we introduce a new constant in the package fluxcd/pkg/apis/event called MetaChangeRequestKey with the value change_request, similar to the existing commit_status key. Then any notifications arriving at notification-controller containing this special key in the event metadata would only be used with this new category of Provider types targeting "change request comments", similar to how we only send commit status updates to the category we already have of commit status providers. The value mapped by this new standard key in the event metadata must contain an identifier for the change request in the respective Git repository configured in the spec.address field of the Provider object.

I propose we address the low-noise requirement by keying the comments. We can do this by embedding a key in the comment body. Then notification-controller would need to build a key from the event and look for the presence of this key in the current list of comments authored by Flux (listing all comments in a change request can be a challenge if there are too many). Then if a comment exists including this key in its body, notification-controller would only update this comment with the content from the latest event at hand. Otherwise, notification-controller would create a new comment including this key in the body.

The comment key could be built similarly to how the commit status ID is built today, which provides uniqueness for tuples of the form (event involved object, provider UID):

// generateCommitStatusID returns a unique ID per cluster based on the Provider UID,
// involved object kind and name.
func generateCommitStatusID(providerUID string, event eventv1.Event) string {
	uidParts := strings.Split(providerUID, "-")
	id := fmt.Sprintf("%s/%s/%s", event.InvolvedObject.Kind, event.InvolvedObject.Name, uidParts[0])
	return strings.ToLower(id)
}

This uniqueness by involved object and provider ensures that we don't run into race conditions when multiple simultaneous events are targeting the same change request but originate from different objects/controllers.

We are currently introducing a way to build custom commit status IDs here by defining a CEL expression in the Provider field spec.commitStatusExpr. A similar field could be introduced for building custom comment keys.

Here's an example of a Provider, Alert and ResourceSet objects (ResourceSetInputProvider omitted for brevity):

apiVersion: notification.toolkit.fluxcd.io/v1beta3
kind: Provider
metadata:
  name: github-pr-comment
  namespace: app-preview
spec:
  type: githubpullrequestcomment
  address: https://github.com/org/app-repo
  secretRef:
    name: github-app-auth
  changeRequestExpr: >
    (event.involvedObject.kind + '/' +
      event.involvedObject.name + '/' +
      event.involvedObject.namespace + '/' +
      provider.metadata.uid.split('-').first().value()
    ).lowerAscii()
---
apiVersion: notification.toolkit.fluxcd.io/v1beta3
kind: Alert
metadata:
  name: github-pr-comment
  namespace: app-preview
spec:
  providerRef:
    name: github-pr-comment
  eventSources:
  - kind: Kustomization
    name: '*'
---
apiVersion: fluxcd.controlplane.io/v1
kind: ResourceSet
metadata:
  name: app
  namespace: app-preview
spec:
  inputsFrom:
  - apiVersion: fluxcd.controlplane.io/v1
    kind: ResourceSetInputProvider
    name: app-pull-requests
  resources:
  - apiVersion: kustomize.toolkit.fluxcd.io/v1
    kind: Kustomization
    metadata:
      name: app-<< inputs.id >>
      namespace: app-preview
      annotations:
        event.toolkit.fluxcd.io/change_request: << inputs.id >>
        event.toolkit.fluxcd.io/preview_url: https://app-<< inputs.id >>.example.com
    ...

Below is an example of the comment notification-controller would post to a pull request. We:

• Sort the metadata keys
• Test if the metadata value starts with http:// or https:// and then do not enclose it with backticks
• Omit the special metadata key change_request

Template

Status for {commentKey}

{event.Severity}: {event.Message}

Metadata

• {key1}: {value1}
  ...
• {keyN}: {valueN}

Template Instance

Status for kustomization/app-preview/app-1234/025ecd77

error: Reconciliation failed terminally due to configuration error

Metadata

• preview_url: https://app-1234.example.com
• revision: refs/heads/pr-branch@sha1:1f3bc4cfa8cb1728e3ecbcbe61b53ec675cbc521

[stefanprodan]

I think the provider name should reflect that this is about posting comments, like it is now, I would think these providers will open/close PRs/MRs.

[matheuscscp]

I think the provider name should reflect that this is about posting comments, like it is now, I would think these providers will open/close PRs/MRs.

Updated provider names and introduced fields.

[stefanprodan]

I'm not for adding templates at this stage, I also think the Provider is the wrong place for defining a template, the Provider is meant to be shared across many Alerts targeting many resources that could have different metadata keys. For templates we would need an RFC that takes into consideration all the provider types. We are now adding the metadata fields as labels in Slack/MSTeam/etc if you use those in the template maybe you don't want the labels anymore, or maybe you want to template the label too. Also adding a template now without having the Event type GA feels wrong to me, if we decide to change the Event fields we'll break the templates.

[matheuscscp]

Okay so if I understand your implicit counter-proposal here you are suggesting for us to choose a fixed format for users, and it must include all the metadata fields. The issue is that notifications for providers like Slack/MSTeam/etc are structured and have native support for appending a metadata map. This is not exactly the case here, we are writing unstructured plain text comments in change requests, but let's give it a try (please feel free to give specific counter-proposals).

We can:

• Sort the metadata keys
• Test if the metadata value starts with http:// or https:// and then do not enclose it with backticks
• Omit the special metadata key change_request

Template

Status for {commentKey}

{event.Severity}: {event.Message}

Metadata

• {key1}: {value1}
  ...
• {keyN}: {valueN}

Template Instance

Status for kustomization/app-preview/app-1234/025ecd77

error: Reconciliation failed terminally due to configuration error

Metadata

• preview_url: https://app-1234.example.com
• revision: refs/heads/pr-branch@sha1:1f3bc4cfa8cb1728e3ecbcbe61b53ec675cbc521

...
(plus other potentially irrelevant metadata fields that may pollute the message)
...

[stefanprodan]

(plus other potentially irrelevant metadata fields that may pollute the message)

Why would other metadata fields be irrelevant on a PR comment but not on a Slack/MSTeams/etc? IMO there is no difference between posting on GitHub/GitLab vs Slack/MSTeams.

[matheuscscp]

Okay I updated the proposal accordingly