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.

Sign up for GitHub

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

Jump to bottom

Introduce the Event v1beta1 API schema #390

Merged
merged 2 commits into from
Nov 8, 2022
Merged

Introduce the Event v1beta1 API schema #390

merged 2 commits into from
Nov 8, 2022

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Nov 7, 2022 •
edited
Loading

The fluxcd/pkg/apis/event package is for formalising how eventing works in Flux. Once this package is released, the fluxcd/pkg/runtime/events and all controllers should switch to it. The Event API comes with helper functions to be used in notification-controller for filtering metadata.

To be used in: fluxcd/notification-controller#435

@stefanprodan stefanprodan added the area/api API related issues and pull requests label Nov 7, 2022
hiddeco
hiddeco reviewed Nov 8, 2022
View reviewed changes
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

By exposing the annotation keys in combination with more generic methods, I think the API would become more versatile and resistant to implementation changes within the domains of the producer and/or consumer.

apis/event/v1beta1/event.go Outdated Show resolved Hide resolved
apis/event/v1beta1/event.go
)

// EventTypeTrace represents a trace event.
const EventTypeTrace string = "Trace"
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between a "trace" event type and a "trace" severity?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used here for mapping https://github.com/fluxcd/pkg/blob/main/runtime/events/recorder.go#L229

apis/event/v1beta1/event.go Outdated
// and trims the API group from the metadata keys.
func (in *Event) StripMetadata() {
group := in.InvolvedObject.GetObjectKind().GroupVersionKind().Group
excludeList := []string{fmt.Sprintf("%s/checksum", group)}
Copy link
Member

@hiddeco hiddeco Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Would expect [%s/]checksum to be exposed as part of the contract. In addition, with RFC-0005 landing soon, we would probably need a %s/digest (and %s/revision) as well.

In extend to this, I am wondering if we can settle on all relevant annotations being prefixed with the GVK domain. As this would be more inline with Kubernetes annotations in general.

Copy link
Member Author

Choose a reason for hiding this comment

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

The prefix borks the alerts, as the keys end up as Stack/Discord/etc labels, that's why we need this strip function.

apis/event/v1beta1/event.go Outdated
}
}

in.Metadata = meta
Copy link
Member

Choose a reason for hiding this comment

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

What would be the intend of the caller / why are we directly mutating the Event?

apis/event/v1beta1/event.go Outdated

// IsCommitStatusUpdate returns true if the event is meant for commit status updates.
func (in *Event) IsCommitStatusUpdate() bool {
if val, ok := in.Metadata["commit_status"]; ok && val == "update" {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of creating a dedicated method for this, and pulling in implementation specific details. I would rather expose the commit_status and offer a generic key/value checker.

Copy link
Member

Choose a reason for hiding this comment

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

In addition to this, I would rename commit_status to commitStatus (leaving backwards compatibility details as an exercise), which is much more inline with both Kubernetes standards and JSON camelCase which the world has settled on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not for introducing any breaking changes in v1beta1. This PR is intended to be first step towards Event consolidation while keeping things as they are

apis/event/v1beta1/event.go Outdated
}

// GetCommitID extracts the Git commit value from the revision string.
func (in *Event) GetCommitID() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Like with IsCommitStatusUpdate, I would make a generic method to retrieve a value from the map and leave the parsing as an exercise for something else. Current logic also conflicts with RFC-0005.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR doesn't try to address the RFC. This PR is intended to be first step towards Event consolidation while keeping things as they are. After RFC-0005 is merged, we could work on v1beta2 and introduce there all these changes.

Copy link
Member

Choose a reason for hiding this comment

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

Following my suggestion would make this much easier?

@hiddeco
Copy link
Member

hiddeco commented Nov 8, 2022 •
edited
Loading

Talked more about this with @stefanprodan. The goal of this PR for now is to at least better document some of the indirect contracts that have been established in the notification-controller around e.g. expected "revision" and "checksum" annotation keys, in combination with generic API utilities to retrieve the values of said annotations.

Not within the scope, but discussed to be followed up at a later time are at least the following things:

  1. Settle on GVK prefixing metadata keys (and how they are filtered out by e.g. notification-controller).

  2. Addressing the "Trace" event type. This type has no value to any arbitrary event recorder implementation, while it indirectly does equal to a "Normal" event type (once it goes past our custom recorder implementation). Making the "Flux event" an imperfect and/or awkward first class citizen.

    One idea would be to make the filtering happen based on an annotation on the K8s event (e.g. group.version.domain/trace: true). Which would both result in reflection of the filter itself on the Kubernetes Event, as it allowing to co-exist with arbitrary recorder implementations.

  3. Dealing with RFC-0005 implications, which will change the revision format and introduce a digest.

  4. Further breaking changes which may result in a better future API.

hiddeco
hiddeco approved these changes Nov 8, 2022
View reviewed changes
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Happy with the scoped outcome of this, thanks @stefanprodan 🙇

@stefanprodan stefanprodan merged commit 5b80083 into main Nov 8, 2022
@stefanprodan stefanprodan deleted the apis-event branch November 8, 2022 12:24
This was referenced Nov 8, 2022
Merged
Merged
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hiddeco hiddeco hiddeco approved these changes

Assignees
No one assigned
Labels
area/api API related issues and pull requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stefanprodan @hiddeco