-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat(repo)!: change allow_<event>
fields to an AllowEvents
struct + DB use integer masking
#314
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #314 +/- ##
==========================================
- Coverage 96.38% 96.11% -0.27%
==========================================
Files 62 67 +5
Lines 6920 7213 +293
==========================================
+ Hits 6670 6933 +263
- Misses 181 205 +24
- Partials 69 75 +6
|
constants/allow_events.go
Outdated
AllowPushBranch = 1 << iota // 00000001 = 1 | ||
AllowPullOpen // 00000010 = 2 | ||
AllowPullEdit // 00000100 = 4 | ||
AllowPullSync // ... | ||
AllowPushTag | ||
AllowDeployCreate | ||
AllowCommentCreate | ||
AllowCommentEdit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note on this: we can't shuffle these at any point if this were to be implemented. So, if a pull_request
event with the action review_requested
is something we support later, then it must go at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like a good thing to add to comments in-line oops, you did already - didn't show in a diff view i was looking at for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we introduce some kind of version bit (byte?) so that the entire thing can be invalidated in the future? For now, just include the bit. Then, the migration logic can be introduced when we bump that version, if we ever do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're using Intl64, and so far this only uses 8 bits, So, the version info could actually be at the end of the 64 bits instead of the beginning. Then, we don't need to reserve any space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My hesitation with including a version is that it would require some sort of DB migration script that updates all existing repos every time we add a new event we support. Part of me wants to just go in and enumerate all possible webhook actions that Vela could theoretically subscribe to and just mark them as not implemented
if they aren't. That way future enhancements would build off a clean order and it wouldn't change unless GitHub itself adds a new event / action worth implementing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Let's leave room for all current GitHub events, but leave a _
for all of the unimplemented ones with a comment saying what that slot is reserved for. Maybe even leave a bit of a gap in each category to add more later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added space for future implementations for pull_request
actions that seem potentially relevant to CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a lot of other possible webhook events:
https://docs.github.com/en/webhooks/webhook-events-and-payloads
How did you pick which of the pull_request
actions deserved an entry here?
After some discussion amongst the @go-vela/admins, we are deciding to:
|
Co-authored-by: Kelly Merrick <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's go to the moon 🌑
Ref: go-vela/community#828
This change lays the groundwork for enhanced event support moving forward by leveraging a bit-shifting iota type for allowed events represented in the database and a new struct for
AllowEvents
in the library.This helps future development of events (and event actions) by not impacting the database. Plus, it allows a clean representation of data for the UI to interpret when the
AllowEvents
struct can be transformed into a nested checklist.