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

Add new watermill handlers that get or refresh entities by properties and call another handler #4545

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Sep 19, 2024

Summary

These will be used in webhook handlers instead of calling the property
service directly. The handlers follow the same base logic, just with
"pluggable" ways of retrieving the entity, converting the entity to
properties and which handler to call next.

The test coverage is not great yet, but I did test the handlers manually
outside the tests I have in another branch and they seemed to work well.

I will be adding more tests as this PR is reviewed.

Related: #4327

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

some unit tests are added in this PR, but I'll add more. Most of the testing has been manual as part of another branch.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@coveralls
Copy link

coveralls commented Sep 19, 2024

Coverage Status

coverage: 53.611% (+0.2%) from 53.46%
when pulling 238f52f on jhrozek:webhook_generic_handler
into 275579a on stacklok:main.

@jhrozek jhrozek force-pushed the webhook_generic_handler branch 2 times, most recently from d5b3a6f to a93a3f4 Compare September 19, 2024 15:06
@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 19, 2024

I split the code into multiple sub-packages with smaller files so it's easier to digest

@jhrozek jhrozek force-pushed the webhook_generic_handler branch 2 times, most recently from ba5a06d to ceaa2f8 Compare September 19, 2024 22:08
@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 19, 2024

There are no functional changes in the latest push, just beefing up the test coverage. Almost there!

JAORMX
JAORMX previously approved these changes Sep 20, 2024
// HandleEntityAndDoMessage is a message that is sent to the entity handler to refresh an entity and perform an action.
type HandleEntityAndDoMessage struct {
Entity TypedProps `json:"entity"`
Owner TypedProps `json:"owner"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Owner can be a little confusing (at first I thought we were conflating GitHub terms again). Can we rename this to originator or begetter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

… and call another handler

These will be used in webhook handlers instead of calling the property
service directly. The handlers follow the same base logic, just with
"pluggable" ways of retrieving the entity, converting the entity to
properties and which handler to call next.

Related: mindersec#4327
@jhrozek jhrozek merged commit bd6e865 into mindersec:main Sep 20, 2024
21 checks passed
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.

3 participants