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 6 commits into
base: main
Choose a base branch
from

Conversation

sebbrandt87
Copy link

@sebbrandt87 sebbrandt87 commented Jan 8, 2024

Add a function to get a more detailed insight into the commit status of e.g. GitLab.
So that you do not need to lookup the first part of the Providers UIDs at all your Flux powered Kubernetes clusters.

@sebbrandt87 sebbrandt87 force-pushed the feat/gitlab-commit-status-event-label-usage branch 2 times, most recently from ecbcebf to 9a7daac Compare January 8, 2024 10:33
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

This change needs to be accompanied by documentation. I'm also not sure how much we're bought into the current format as people might already be relying on it as it is right now. @stefanprodan?

internal/notifier/util.go Outdated Show resolved Hide resolved
@@ -59,10 +59,29 @@ 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 ?

Signed-off-by: Brandt, Sebastian (ITO-PC) <sebastian.brandt@tchibo.de>
@sebbrandt87 sebbrandt87 force-pushed the feat/gitlab-commit-status-event-label-usage branch from 9a7daac to fa99576 Compare January 8, 2024 22:04
docs/spec/v1beta3/providers.md Outdated Show resolved Hide resolved
@@ -1474,6 +1474,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

docs/spec/v1beta3/providers.md Outdated Show resolved Hide resolved
docs/spec/v1beta3/providers.md Show resolved Hide resolved
sebbrandt87 and others added 4 commits January 11, 2024 16:59
Co-authored-by: Max Jonas Werner <makkes@users.noreply.github.com>
Signed-off-by: Sebastian Brandt <sebbrandt@mailbox.org>
Co-authored-by: Max Jonas Werner <makkes@users.noreply.github.com>
Signed-off-by: Sebastian Brandt <sebbrandt@mailbox.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants