Skip to content
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

Add has_successful_status predicate #141

Merged
merged 13 commits into from
Dec 6, 2019

Conversation

sjrand
Copy link
Contributor

@sjrand sjrand commented Dec 5, 2019

Continues the work started by @pilbot in #139. I don't have permission to push to that branch, thus the new one.

Fixes #102.

Copy link
Member

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

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

This is almost ready, but I think there's one more issue with the PR listing. Once that's resolved it should be good to merge.

@@ -114,12 +115,12 @@ func (h *Status) processOthers(ctx context.Context, event github.StatusEvent) er

// In practice, there should only be one or two open PRs for a given commit. In exceptional cases, if there are
// more than 100 open PRs, only process the most recent 100.
prs, _, err := client.PullRequests.List(
prs, _, err := client.PullRequests.ListPullRequestsWithCommit(
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I didn't know this endpoint existed in the v3 API. Per the docs, it looks like it returns both closed and open PRs:

Lists all pull requests containing the provided commit SHA, which can be from any point in the commit history. The results will include open and closed pull requests.

I think the 100 PR limit is probably still safe in practice (although the comment is inaccurate now), but we do need to filter the results to only evaluate open PRs in the loop on L132.

@@ -137,6 +138,9 @@ func (h *Status) processOthers(ctx context.Context, event github.StatusEvent) er
})
if err != nil {
evaluationFailures++
logger.Error().Err(err).Msgf("failed to evaluate pull request '%d' for SHA '%s'", pr.GetNumber(),
Copy link
Member

Choose a reason for hiding this comment

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

style: Failed to .... In this codebase, log message usually start with a captial letter, errors with a lowercase letter.

@bluekeyes bluekeyes merged commit 883b291 into palantir:develop Dec 6, 2019
@sjrand sjrand deleted the has-status-passed branch December 9, 2019 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Approve if other status check has passed
3 participants