Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add function to create a more speaking id for commit status #705

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
28 changes: 28 additions & 0 deletions docs/spec/v1beta3/providers.md
Original file line number Diff line number Diff line change
Expand Up @@ -1497,6 +1497,34 @@ spec:
name: flux-system
```

##### Construction of Git Commit Status ID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explain here what the commit status ID actually is and how it can be leveraged.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it might be enough to link to GitHub and GitLab related API endpoints - as this documentation stays up2date? @makkes

GitHub: https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28
GitLab: https://docs.gitlab.com/ee/api/commits.html#set-the-pipeline-status-of-a-commit


By default the commit status ID is constructed through the `eventSources.kind`, `eventSources.name` and the
[`UID`](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids) of the `Provider`.

To generate a descriptive prefix, instead of relying on the `UID` of the `Provider`, `eventMetadata` can be set.

```yaml
apiVersion: notification.toolkit.fluxcd.io/v1beta3
kind: Alert
metadata:
name: github-status
namespace: flux-system
spec:
providerRef:
name: github-status
eventMetadata:
app.kubernetes.io/env: "production"
app.kubernetes.io/cluster: "my-cluster"
app.kubernetes.io/region: "us-east-1"
makkes marked this conversation as resolved.
Show resolved Hide resolved
eventSources:
- kind: Kustomization
name: flux-system
```

In this case the Git Commit Status ID will be extended by a suffix which is a combination of `app.kubernetes.io/`
string sorted labels. The example above constructs a suffix as follows: "/my-cluster/production/us-east-1".

#### GitHub

When `.spec.type` is set to `github`, the referenced secret must contain a key called `token` with the value set to a
Expand Down
30 changes: 29 additions & 1 deletion internal/notifier/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"crypto/sha1"
"encoding/base64"
"fmt"
"sort"
"strings"
"unicode"
"unicode/utf8"
Expand Down Expand Up @@ -59,10 +60,37 @@ func formatNameAndDescription(event eventv1.Event) (string, string) {
// 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])
metadataLabelsSuffix := createMetadataLabelsSuffix(event)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the potential to result in a very long string which is undesirable. We need to think on a way to constrain this to a certain length.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, it might be an option to change eventMetadata keys accepted for building an extended commit status ID? @makkes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any suggestion how to proceed @makkes @stefanprodan ?

id := fmt.Sprintf("%s/%s/%s%s", event.InvolvedObject.Kind, event.InvolvedObject.Name, uidParts[0], metadataLabelsSuffix)
return strings.ToLower(id)
}

// createMetadataLabelsSuffix returns a concated string depending on app.kubernetes.io/,
// prefixed common labels https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/
func createMetadataLabelsSuffix(event eventv1.Event) string {
metadataKeys := make([]string, 0, len(event.Metadata))
metadataLabels := make([]string, 0)
prefixedMetadataLabelsSuffix := ""

// order Metadata keys
for labelKey := range event.Metadata {
metadataKeys = append(metadataKeys, labelKey)
}
sort.Strings(metadataKeys)
// iteratore over ordered Metadata keys
for _, key := range metadataKeys {
if strings.Contains(key, "app.kubernetes.io/") {
metadataLabels = append(metadataLabels, event.Metadata[key])
}
}
metadataLabelsSuffix := strings.Join(metadataLabels, "/")
if len(metadataLabelsSuffix) > 0 {
prefixedMetadataLabelsSuffix = fmt.Sprintf("/%s", metadataLabelsSuffix)
}

return strings.ToLower(prefixedMetadataLabelsSuffix)
}

func splitCamelcase(src string) (entries []string) {
// don't split invalid utf8
if !utf8.ValidString(src) {
Expand Down
32 changes: 32 additions & 0 deletions internal/notifier/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,38 @@ func TestUtil_GenerateCommitStatusID(t *testing.T) {
},
want: "kustomization/gitops-system/0c9c2e41",
},
{
name: "test with non related metadata event case",
providerUID: "0c9c2e41-d2f9-4f9b-9c41-bebc1984d67a",
event: eventv1.Event{
Metadata: map[string]string{
"foobar/batz": "test",
},
InvolvedObject: corev1.ObjectReference{
Kind: "Kustomization",
Name: "gitops-system",
},
Reason: "ApplySucceeded",
},
want: "kustomization/gitops-system/0c9c2e41",
},
{
name: "test with related metadata event case",
providerUID: "0c9c2e41-d2f9-4f9b-9c41-bebc1984d67a",
event: eventv1.Event{
Metadata: map[string]string{
"app.kubernetes.io/cluster": "testcluster",
"app.kubernetes.io/env": "test",
"app.kubernetes.io/region": "europe-west4",
},
InvolvedObject: corev1.ObjectReference{
Kind: "Kustomization",
Name: "gitops-system",
},
Reason: "ApplySucceeded",
},
want: "kustomization/gitops-system/0c9c2e41/testcluster/test/europe-west4",
},
}

for _, tt := range statusIDTests {
Expand Down