-
Notifications
You must be signed in to change notification settings - Fork 29
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)!: add pending approval timeout #1227
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1227 +/- ##
==========================================
- Coverage 56.91% 56.84% -0.08%
==========================================
Files 599 602 +3
Lines 32904 33036 +132
==========================================
+ Hits 18727 18778 +51
- Misses 13541 13620 +79
- Partials 636 638 +2
|
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
// publish the build to the queue | ||
go Enqueue( | ||
context.WithoutCancel(c.Request.Context()), | ||
queue.FromGinContext(c), | ||
database.FromContext(c), | ||
item, | ||
b.GetHost(), | ||
) |
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.
thoughts on separating the logic from the action taken?
// determine whether to send compiled build to queue
do the determining, return bool error
then gatekeep or enqueue afterwards?
seems cleaner that way, like how we separate logic and API response writer code in other spots
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.
returning bool based on some logic might also make the function more testable
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.
I like the idea. There's a few spots within TrafficBuild and gatekeepBuild that leverage contextual services, which may make testing a little tricky. But I'll see what I can do.
Give repos a new setting called
approval_timeout
which will set builds that have been sitting in apending approval
state to a failure if they've been unapproved for the given time.Also applied some approval logic to restarts now that we store
fork
on builds.