-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fix builds status script #2285
Fix builds status script #2285
Conversation
Get a batch of the last 1000 builds, and filter on those ourselves instead of calling the cloudbuild API to filter for us. The cloudbuild API filtering API does not work with our number of builds.
TBR to fix things. |
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 with some minor fixes/questions. Thanks for fixing this.
|
||
builds = builds.get(tag) | ||
if not builds: | ||
return None |
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.
Do we need to add any prints here for later easy debugging. Should this ever happen. Same for 2 continue below.
next_page_token = None | ||
|
||
while True: | ||
page_size = min(BUILDS_PAGE_SIZE, MAX_BUILD_RESULTS - len(ungrouped_builds)) |
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.
Why if len(ungrouped_builds) > MAX_BUILD_RESULTS, this will go negative and that negative page size will be used. What is the need to restrict max results ?
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 guess the negative shouldn't happen here as per lines 130-131
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.
right, it shouldn't happen per the exist condition.
projectId='oss-fuzz', pageSize=page_size, pageToken=next_page_token)) | ||
|
||
if not 'builds' in response: | ||
print >> sys.stderr, 'Invalid response', response |
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.
may add "when trying to fetch build list"
|
||
ungrouped_builds.extend(response['builds']) | ||
if len(ungrouped_builds) >= MAX_BUILD_RESULTS: | ||
break |
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.
add a print like "Build limit reached, skipping rest of builds ?"
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.
it's not feasible to fetch all builds, which is why we are limiting things here. so, this isn't an error condition :)
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
next_page_token = None | ||
|
||
while True: | ||
page_size = min(BUILDS_PAGE_SIZE, MAX_BUILD_RESULTS - len(ungrouped_builds)) |
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 guess the negative shouldn't happen here as per lines 130-131
Get a batch of the last 2000 builds, and filter on those ourselves
instead of calling the cloudbuild API to filter for us. The cloudbuild
API filtering API does not work with our number of builds.
Fixes #2263