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 workflow to comment on new PRs #5544

Closed

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented Nov 7, 2024

this is a follow up to #5489 and a different approach to address the issue: when contributors raise a PR with our project they were often struggle with our CI checks, since there are many of them and they are often not aware of the capabilities we have to auto-fix the most common issues. At the same time we want to let them know that they do not have to worry about failed CI checks, since we can help them with addressing them (often I actually prefer fixing them myself before merging a PR).

Currently a PR submitter will not get this information if they do not read deep into our contribution guidelines. So instead of leaving them lost when they submit a PR, I suggest 2 solutions (we can have both):

  1. Put an automated comment under each PR that shares this information with the PR submitter (this PR)
  2. Update the PULL_REQUEST_TEMPLATE to hold this information (will submit another PR for that)

Personally I would prefer having both options, but the downside of (1) is that it will comment on every new PR. There are ways to reduce that to first time contributors or we could only comment if CI checks fail, but I wanted to put this out for discussion first.

Here is a comment how it would look like:

svrnm#189 (comment)

@svrnm svrnm requested a review from a team as a code owner November 7, 2024 11:34
Copy link
Contributor

@tiffany76 tiffany76 left a comment

Choose a reason for hiding this comment

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

Just aligning with the changes I suggested in the PR template language. LGTM

Comment on lines +26 to +28
We will run some checks on your PR. If they fail, take a look in to our [guide on pr-checks](https://opentelemetry.io/docs/contributing/pr-checks/).
Most issues can be resolved by running \`npm run fix:all\` locally and pushing the changes.
But please note that it is not your responsibility to fix all the issues. We will help you with that.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We will run some checks on your PR. If they fail, take a look in to our [guide on pr-checks](https://opentelemetry.io/docs/contributing/pr-checks/).
Most issues can be resolved by running \`npm run fix:all\` locally and pushing the changes.
But please note that it is not your responsibility to fix all the issues. We will help you with that.
We run checks on all PRs. If a check fails on your PR, look for the fix in our [guide](https://opentelemetry.io/docs/contributing/pr-checks/).
Most issues can be resolved by running \`npm run fix:all\` locally and pushing the changes.
But please note that it is not your responsibility to fix these issues. We will help you.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

See inline comments.

This might make things verbose (given that it adds a comment to all PRs), but it's worth a try. Thanks for putting this together.

repo: context.repo.repo,
body: `Hello 👋! Thanks for opening a pull request.

Please make sure to follow the [contribution guidelines](https://opentelemetry.io/docs/contributing/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Please make sure to follow the [contribution guidelines](https://opentelemetry.io/docs/contributing/).
Make sure that you are following our [contribution guidelines](https://opentelemetry.io/docs/contributing/).


Please make sure to follow the [contribution guidelines](https://opentelemetry.io/docs/contributing/).

We will run some checks on your PR. If they fail, take a look in to our [guide on pr-checks](https://opentelemetry.io/docs/contributing/pr-checks/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We will run some checks on your PR. If they fail, take a look in to our [guide on pr-checks](https://opentelemetry.io/docs/contributing/pr-checks/).
We run some checks on your PR. For guidance on what to do when checks fail, see [PR checks](https://opentelemetry.io/docs/contributing/pr-checks/).


We will run some checks on your PR. If they fail, take a look in to our [guide on pr-checks](https://opentelemetry.io/docs/contributing/pr-checks/).
Most issues can be resolved by running \`npm run fix:all\` locally and pushing the changes.
But please note that it is not your responsibility to fix all the issues. We will help you with that.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
But please note that it is not your responsibility to fix all the issues. We will help you with that.
We maintainers can sometimes help you resolve issues with your PR, but fixing them is up to you.

Copy link
Member Author

@svrnm svrnm Nov 8, 2024

Choose a reason for hiding this comment

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

I phrased it that way because depending on the issue I rather fix it myself vs leaving it to an (unexperienced) contributor. It can be very frustrating for them to get the checks all satisfied. They create a lot of notifications for us and also a lot of CI check runs that clog the github workers.

A different wording would be:

Do not worry about the status of the CI checks after submitting your PR. We will provide you with guidance, when and how to address them.

@svrnm
Copy link
Member Author

svrnm commented Nov 8, 2024

This might make things verbose (given that it adds a comment to all PRs), but it's worth a try. Thanks for putting this together.

Yes, what I want to do is double checking if commenting if and only if at least one check fails would be the better choice, or even something like "hey this and that checked failed, you can run this and that command to fix it", but depending on the complexity I would start with this one and evolve from there.

Thanks for the initial feedback

@svrnm
Copy link
Member Author

svrnm commented Nov 11, 2024

I will revisit this once again, I have a few more ideas to make this less spammy. Let's finish #5545 first, it's the best starting point 👍

@svrnm svrnm closed this Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants