-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix handling of 405 merge in progress #13770
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 handling of 405 merge in progress #13770
Conversation
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
.ci/magician/github/get.go
Outdated
@@ -116,7 +116,7 @@ func (gh *Client) GetCommitMessage(owner, repo, sha string) (string, error) { | |||
} `json:"commit"` | |||
} | |||
|
|||
err := utils.RequestCall(url, "GET", gh.token, &commit, nil) | |||
err := utils.RequestCallSilent(url, "GET", gh.token, &commit, nil) |
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 does this need to be silent? We generally want to log all of these requests for debugging purposes.
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.
Thomas removed the printing of the result for POST requests because this API outputs a lot. I am pretty sure we are listing a lot of commits so I can understand.
I added the printing of the POST back but thomas was saying that this caused the test output to be too long. I don't think we should be testing this in a presubmit anyways but this is just to unblock adding back the POST result to console
Co-authored-by: Stephen Lewis (Burrows) <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
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 would somewhat prefer that we fix the unit test to not call the API rather than remove logging, and I have a slight concern that this extra method will live longer than we want it to - but I don't currently feel strongly enough to block merging this.
Co-authored-by: Stephen Lewis (Burrows) <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
Okay, I changed the testcase instead. Note that this will mean the delete branch operation might be a bit noisy in cloud build. for-examples: |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
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.
ack re: noise.
ffdfe9e
Co-authored-by: Stephen Lewis (Burrows) <[email protected]>
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.