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

Update Event interface code doc #825

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

chrisdoherty4
Copy link
Member

The Event interface is implemented by all events. The code docs were not clear on how a consumer leverages the Event interface to handle the event (for example transport the event).

This code doc clarifies the intended usage of the Event interface.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #825 (80fd36f) into main (fb0c11c) will not change coverage.
Report is 3 commits behind head on main.
The diff coverage is n/a.

❗ Current head 80fd36f differs from pull request most recent head 946b93c. Consider uploading reports for the commit 946b93c to get more accurate results

@@           Coverage Diff           @@
##             main     #825   +/-   ##
=======================================
  Coverage   51.16%   51.16%           
=======================================
  Files          33       33           
  Lines        1456     1456           
=======================================
  Hits          745      745           
  Misses        665      665           
  Partials       46       46           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -7,7 +7,17 @@ import "context"
// Name is a unique name identifying an event.
type Name string

// Event is a recordable event.
// Event is a recordable event. Each event in the event package implements this interface. Consumers
Copy link
Member

Choose a reason for hiding this comment

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

This is great, thanks. A few suggestions for things to add that I think would be helpful for implementors.

  • what "recordable event" is, means, and when/where events are recorded (i believe its just in the agent logic).
  • transports implementations are responsible for sending the event.
  • concrete event types will contain more context that can be used when sending an event.

Copy link
Member Author

@chrisdoherty4 chrisdoherty4 Oct 26, 2023

Choose a reason for hiding this comment

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

I can add something for the first 2. Isn't the third point covered by the comment on how to use and the example?

Copy link
Member Author

@chrisdoherty4 chrisdoherty4 Nov 2, 2023

Choose a reason for hiding this comment

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

I've tweaked this a little but I decided not to make any mention of transport because thats 1 of possibly more consumers. I think the documentation in this package needs to be tight and relevant to the package itself.

We could add documentation elsewhere that strings everything together but I don't think its appropriate for this interface.

Signed-off-by: Chris Doherty <[email protected]>
@chrisdoherty4 chrisdoherty4 added the ready-to-merge Signal to Mergify to merge the PR. label Nov 2, 2023
@mergify mergify bot merged commit c3fe06a into tinkerbell:main Nov 2, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants