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

build: Post pull request comments on build check failure #218

Closed
wants to merge 1 commit into from

Conversation

fghaas
Copy link
Contributor

@fghaas fghaas commented Aug 16, 2023

Without this change, when a PR results in a build failure, the onus is on the PR submitter to find the failed build, and the corresponding build log. That is somewhat tedious and not particularly intuituive.

Instead, use a separate GitHub Action that posts a comment back to the PR thread, containing a direct link to the failed build log. This should hopefully make it easier for PR submitters to find out what exactly their submitted change breaks.

Reference:
quipper/comment-failure-action#6

@fghaas fghaas force-pushed the add-comments-on-pr branch from a42802d to b1a4f97 Compare August 16, 2023 10:36
# Post a comment back on a PR thread, in case an associated workflow
# failed. Note that comment-failure-action is a no-op in case the
# workflow was not invoked in response to a PR. In other words, this
# action only does creates a comments for workflows configured with
Copy link
Contributor

@colder-is-better colder-is-better Aug 16, 2023

Choose a reason for hiding this comment

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

Suggested change
# action only does creates a comments for workflows configured with
# action only creates comments for workflows configured with

Copy link
Contributor

@colder-is-better colder-is-better left a comment

Choose a reason for hiding this comment

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

A small nit is all I have!

Without this change, when a PR results in a build failure, the onus is
on the PR submitter to find the failed build, and the corresponding
build log. That is somewhat tedious and not particularly intuituive.

Instead, use a separate GitHub Action that posts a comment back to the
PR thread, containing a direct link to the failed build log. This
should hopefully make it easier for PR submitters to find out what
exactly their submitted change breaks.

References:
https://github.com/quipper/comment-failure-action/
quipper/comment-failure-action#6
@fghaas fghaas force-pushed the add-comments-on-pr branch from b1a4f97 to f283b50 Compare August 16, 2023 11:48
@fghaas
Copy link
Contributor Author

fghaas commented Aug 16, 2023

Thanks for catching that; I've fixed that nit. Also, please note that this is one of those things that we can only actually test once they've landed in main, so apologies in advance in case this happens to break anything.

Copy link
Contributor

@colder-is-better colder-is-better left a comment

Choose a reason for hiding this comment

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

LGTM!

@colder-is-better
Copy link
Contributor

Thanks for catching that; I've fixed that nit. Also, please note that this is one of those things that we can only actually test once they've landed in main, so apologies in advance in case this happens to break anything.

Makes sense, yes.

@fghaas fghaas closed this Aug 16, 2023
fghaas added a commit to fghaas/cleura-docs that referenced this pull request Aug 16, 2023
The workflow introduced in
citynetwork#218, intended to facilitate
pull requests by adding a comment in case the PR produced any build
errors, failed on its first test. A build failure on
citynetwork#217 did cause the action to
fire, but it immediately failed with the following message:

Run quipper/[email protected]
No open pull requests found with packettoobig-patch-1 branch

Since a pull request from that branch obviously did exist, it follows
that the action isn't working as intended, so drop it and rely on
per-user build notifications, set via
https://github.com/settings/notifications, instead.
@fghaas
Copy link
Contributor Author

fghaas commented Aug 16, 2023

Oddly enough, GitHub marked this one as Closed (as if it hadn't been merged). For the record, this was merged in f283b50, and then backed out again in e6a3ca1 (#219).

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.

2 participants