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

Wrap EventRecorder #1519

Closed
8 tasks done
lindnerby opened this issue May 6, 2024 · 0 comments · Fixed by #1584
Closed
8 tasks done

Wrap EventRecorder #1519

lindnerby opened this issue May 6, 2024 · 0 comments · Fixed by #1584
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@lindnerby
Copy link
Member

lindnerby commented May 6, 2024

Description

Currently KymaReconciler uses the dependency of EventRecorder directly, which exposes all possibilities of sending events.
To limit the chance of introducing more calls to r.Event() and to introduce error truncation into the warning event payload generation, we want to wrap the consumption of the EventRecorder.

Reasons

Testability, Reduce Error message size for smaller events in etcd

Acceptance Criteria

  • Introduce new interface dependency to KymaReconciler called Event as embedded struct
  • Events implementation uses the "k8s. io/ client-go/ tools/ record EventRecorder" instead
  • Provide two methods Normal() and Warning() and replace existing occurrences of r.Event(*) (see below) by calling these two methods instead, e.g. r.Event.Warning(kyma, reason, err)

Image

  • The Warning() event function will again accept an error parameter in its signature, but it truncates the length of err.Error() (refine!)
  • Think about separate implementations of KymaEvent, WatcherEvent, etc. if they make sense
  • Since Event implementation will be initialized with mgr.GetEventRecorderFor(shared.OperatorName) the pointer to the dependency can be shared on setup between all consumers, which makes the sharing the event recorder via context obsolete, so the adapter pkg can be deleted as well
  • For integration test there will be an implementation of the Events interface that provides initialization with k8sManager.GetEventRecorderFor(shared.OperatorName) as seen here
  • Remove unused KymaReconciler dependency

Feature Testing

Unit tests

Testing approach

The Events interface implementation can be fully unit tested.

Attachments

No response

@lindnerby lindnerby added the kind/feature Categorizes issue or PR as related to a new feature. label May 6, 2024
@lindnerby lindnerby changed the title Feature Title Extract EventRecorder May 6, 2024
@lindnerby lindnerby changed the title Extract EventRecorder Wrap EventRecorder May 6, 2024
@lindnerby lindnerby self-assigned this May 22, 2024
@lindnerby lindnerby linked a pull request May 28, 2024 that will close this issue
@lindnerby lindnerby assigned nesmabadr and unassigned lindnerby May 28, 2024
@jeremyharisch jeremyharisch assigned lindnerby and unassigned nesmabadr Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants