Skip to content

Check for full payload in case of failure to get PR base ref#19112

Closed
gnossen wants to merge 4 commits intogrpc:masterfrom
gnossen:instrument_for_gh_failures
Closed

Check for full payload in case of failure to get PR base ref#19112
gnossen wants to merge 4 commits intogrpc:masterfrom
gnossen:instrument_for_gh_failures

Conversation

@gnossen
Copy link
Copy Markdown
Contributor

@gnossen gnossen commented May 22, 2019 •
edited
Loading

Experiment to track down the root cause of #18968.

This is not yet ready to be merged.

In our setup scripts, we're piping the results of curl straight to jq, who is producing the string "null\0a". This throws away the JSON payload returned from Github without us ever getting to read it. This PR rectifies that.

@gnossen gnossen added disposition/DO NOT MERGE area/build release notes: no Indicates if PR should not be in release notes labels May 22, 2019
@gnossen gnossen mentioned this pull request May 22, 2019
Merged
@gnossen
Copy link
Copy Markdown
Contributor Author

gnossen commented May 22, 2019

@nicolasnoble Build log confirms the problem is rate limiting:

++ echo 'PR info did not contain the base ref.'
PR info did not contain the base ref.
++ echo '{
  "message": "API rate limit exceeded for 108.177.31.30. (But here'\''s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
  "documentation_url": "https://developer.github.com/v3/#rate-limiting"
}'
{
  "message": "API rate limit exceeded for 108.177.31.30. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
  "documentation_url": "https://developer.github.com/v3/#rate-limiting"
}

@gnossen gnossen mentioned this pull request May 22, 2019
Closed
jtattermusch
jtattermusch approved these changes May 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM. thanks for helping track this down.

@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented May 23, 2019

it feels like needing to check for the base branch ourselves is pretty clumsy - I filed an internal kokoro feature request for kokoro to provide that info (so that we don't need to resort to fragile scripts to get the info).

@jtattermusch jtattermusch mentioned this pull request May 23, 2019
Merged
@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented May 23, 2019

Superseded by #19120.

@gnossen gnossen closed this May 23, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nicolasnoble nicolasnoble Awaiting requested review from nicolasnoble

1 more reviewer

@jtattermusch jtattermusch jtattermusch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/build release notes: no Indicates if PR should not be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gnossen @jtattermusch