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

Make github webhook easier to extend with new events for auto registration. #3346

Merged
merged 6 commits into from
May 30, 2024
Merged

Make github webhook easier to extend with new events for auto registration. #3346

merged 6 commits into from
May 30, 2024

Conversation

blkt
Copy link
Contributor

@blkt blkt commented May 16, 2024 •
edited
Loading

Summary

The idea is to make it easier to extend github webhook with events that are tied to more than one repository, namely "installation" and "installation_repositories", which are necessary for repo auto registration.

Fixes #3359

Change Type

Mark the type of change your PR introduces:

  • 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

Added several unit tests, plus I additionally tested it manually on a local environment.

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.

@blkt blkt self-assigned this May 16, 2024
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Member

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Member

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Member

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Member

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Member

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

stacklokbot
stacklokbot reviewed May 17, 2024
View reviewed changes
Copy link
Member

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 17, 2024
View reviewed changes
Copy link
Member

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@blkt blkt force-pushed the webhook-refactor branch 2 times, most recently from a774c93 to df5874c Compare May 21, 2024 11:03
@coveralls
Copy link

coveralls commented May 21, 2024 •
edited
Loading

Coverage Status

coverage: 52.365% (+0.3%) from 52.109%
when pulling a01f93c on webhook-refactor
into 5065d65 on main.

@blkt blkt force-pushed the webhook-refactor branch 2 times, most recently from 2e7734c to 61b87ce Compare May 21, 2024 12:56
@blkt blkt marked this pull request as ready for review May 21, 2024 12:56
@blkt blkt requested a review from a team as a code owner May 21, 2024 12:56
@blkt blkt force-pushed the webhook-refactor branch 3 times, most recently from 9247a37 to 92844e7 Compare May 22, 2024 12:31
@blkt blkt marked this pull request as draft May 23, 2024 09:23
@blkt blkt force-pushed the webhook-refactor branch 4 times, most recently from 35b3d14 to ffe3c63 Compare May 24, 2024 12:04
@blkt blkt marked this pull request as ready for review May 24, 2024 12:05
@blkt blkt force-pushed the webhook-refactor branch 2 times, most recently from 934932d to 5252307 Compare May 26, 2024 14:56
@blkt blkt force-pushed the webhook-refactor branch 6 times, most recently from a26fafd to 32e8ac3 Compare May 29, 2024 08:22
jhrozek
jhrozek reviewed May 29, 2024
View reviewed changes
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

Still reading the patchset, just want to submit some comments I had pending so the review keeps happening from both sides. I like the code so far!

internal/controlplane/handlers_githubwebhooks.go Outdated Show resolved Hide resolved
internal/controlplane/handlers_githubwebhooks.go Show resolved Hide resolved
internal/controlplane/handlers_githubwebhooks.go Outdated Show resolved Hide resolved
@jhrozek
Copy link
Contributor

jhrozek commented May 29, 2024

For the record, I like the code, I'm just doing some manual testing before finally acking.

jhrozek
jhrozek approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

Manual tests seem to work fine and the code looks good, too.

I think @JAORMX needs to take a look as well because currently he sets changes requested.

blkt added 6 commits May 29, 2024 16:43
@blkt
Make github webhook easier to extend with new events for auto registr…
bff5a4c 
…ation.

The idea is to make it easier to extend github webhook with events
that are tied to more than one repository, namely "installation" and
"installation_repositories", which are necessary for repo auto
registration.

Fixes #3359
@blkt
More uniform error messages and some comments.
fe7e316
@blkt
Added case for push events.
eb52cc8 
To avoid duplicating more code, repoEntity wrapper struct was
added. This struct exposes the fields we're interested in for structs
representing repositories.
@blkt
Added case for pull request event.
49876c1
@blkt
Added Installation Info Wrapper.
52336ed 
This decouples processing of "installation" events from the production
of the message for further processing. Implementation is inspered to
EntityInfoWrapper.
@blkt
Minor cleanup.
a01f93c
@blkt blkt requested a review from JAORMX May 29, 2024 14:45
@blkt blkt merged commit 300d857 into main May 30, 2024
20 checks passed
@blkt blkt deleted the webhook-refactor branch May 30, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stacklokbot stacklokbot stacklokbot left review comments

@JAORMX JAORMX JAORMX approved these changes

@jhrozek jhrozek jhrozek approved these changes

Assignees

@blkt blkt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor GitHub webhook handler to have a case for each event of interest
5 participants
@blkt @coveralls @jhrozek @JAORMX @stacklokbot