-
Notifications
You must be signed in to change notification settings - Fork 147
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
RateLimitBranchProperty
: Fix checking queue for other items tied to the same task
#383
RateLimitBranchProperty
: Fix checking queue for other items tied to the same task
#383
Conversation
… to the same task There is no need for a for loop when only checking the first item would be enough.
RateLimitBranchProperty
: simplify checking queue for other items tied to the same task
RateLimitBranchProperty
: simplify checking queue for other items tied to the same taskRateLimitBranchProperty
: Fix checking queue for other items tied to the same task
@@ -537,7 +537,7 @@ public CauseOfBlockage canRun(Queue.Item item) { | |||
return null; | |||
} | |||
for (Queue.Item i : items) { | |||
if (i.getId() < item.getId()) { | |||
if (i.getInQueueSince() < item.getInQueueSince()) { |
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.
Now printing
foo/master with queue id 4 blocked by queue id 3 was first
as expected.
I suspect you need a short pause between branch-api-plugin/src/test/java/jenkins/branch/RateLimitBranchPropertyTest.java Lines 250 to 253 in f9e35c4
<= while checking (&& ) that the Item s or their id s are != .
|
The latter makes more sense to me since in theory this condition can still happen (if jobs are triggered via a script). |
getId
is not comparable, butgetInQueueSince
is.This also adds a
LoggerRule
to the test class to make the test output more readable.cc @jglick
Testing done
Submitter checklist