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

Customize PR instructions based on files modified #6685

Merged

Conversation

anthonypz
Copy link
Member

Fixes #5182

What changes did you make?

  • This change grabs a list of modified files submitted in the Pull Request (using the global github and context variables).
  • Wrote logic to only include the Pull Request and Contributor instructions based on what files are modified in the PR.

Why did you make the changes (we will use this info to test)?

  • The changes were made so that PR instructions not relevant to the Pull Request are omitted, eliminating confusion for contributors.

@HackforLABot HackforLABot added this to PR Needs review (Automated Column, do not place items here manually) in Project Board Apr 19, 2024
@github-actions github-actions bot added role: back end/devOps Tasks for back-end developers Complexity: Large Feature: Board/GitHub Maintenance Project board maintenance that we have to do repeatedly size: 3pt Can be done in 13-18 hours labels Apr 19, 2024
@t-will-gillis t-will-gillis self-requested a review April 22, 2024 02:03
Copy link
Member

@t-will-gillis t-will-gillis left a comment

Choose a reason for hiding this comment

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

Hey @anthonypz Good job with this PR so far. I can follow your logic and understand how this should be working; however, when I test it in my repo the message is not being posted.

Here is one test issue I ran in my repo. As can be seen, the bot is posting [object Promise] instead of the message. I believe that the code is mostly working but something is breaking down.

Since this issue directly relates to a GitHub Action, this issue really needs to be tested as a GHA which means you will need to set up the testing environment in your own repo to demonstrate the GHA works. This sheet is probably the best starting point as well as the documents listed on #5182.

I can help you once you get set up- feel free to post questions on this issue and on Slack.

Thanks for working on this- as I said, I think the code is mostly there.

Project Board automation moved this from PR Needs review (Automated Column, do not place items here manually) to test-pending-approval (Automated Column, do not place items here manually) Apr 22, 2024
@t-will-gillis
Copy link
Member

t-will-gillis commented Apr 24, 2024

Hey @anthonypz Great job! I can confirm this is working now: without CONTRIBUTING.md the PR review message is the same as previous, with only CONTRIBUTING.md the PR review message only refers to that file, and with CONTRIBUTING.md and another file both PR messages are shown.

I would like to run a couple of formatting questions by @roslynwythe that are beyond the scope of the original issue. I think that the message for CONTRIBUTING.md only is a little awkward with the heavy line at the beginning and with the omission of the "Want to review this pull request..." message:

Screenshot 2024-04-24 152335
  • I believe that for any message, we would like to include the following, initial statement. To accomplish this, the statement would either be a third .md file in the /pr-instructions/ folder or possibly(?) hardcoded into create-instructions.js:

Want to review this pull request? Take a look at this documentation for a step by step guide.

  • Then I would opt for either removing the ---------------- from line 3 of pr-instructions-contrib-template.md to remove the heavy line completely, or alternately inserting + ----------- + before line 5 of pr-instructions-template.md so those both messages include the heavy separation. (Note that --------------- can be ---)

@anthonypz
Copy link
Member Author

Hey @t-will-gillis, thanks for reviewing my changes. Good catch - I agree that your suggested changes make more sense. I'd be happy to implement them as part of this pull request as soon as we know what those changes should look like.

@roslynwythe
Copy link
Member

@t-will-gillis I agree that your suggested changes are worthwhile. Thank you

@t-will-gillis
Copy link
Member

t-will-gillis commented Apr 27, 2024

Hey @anthonypz Let's go ahead with this then. In the end the comments for the three situations (with CONTRIBUTING.md and another file(s), with only CONTRIBUTING.md, and with only other files) would look similar to this demo issue.

--> Feel free to make suggestions if you see something...

To confirm:

  • The following message should appear at the top for all situations
    • For consistency, this could be in a file named pr-instructions-header.md
    • This is definitely out of scope and could be done in a future issue, if ever: If you wanted to figure out the logic, I think it would be possible to have all three messages in the same .md file.
Want to review this pull request? Take a look at [this documentation](https://github.com/hackforla/website/wiki/How-to-Review-Pull-Requests) for a step by step guide!  
  • pr-instructions-template.md
    • would no longer have the above statement
    • would add --- between lines 2 and 5 similar to pr-instructions-contrib-template.md
  • pr-instructions-contrib-template.md
    • essentially ok, could replace line 3 with ---

Logic in create-instructions.js as needed to create the appropriate messages.

Thank you for doing this work beyond the scope of the original issue!

@anthonypz
Copy link
Member Author

Thanks for the detailed instructions and providing a demo issue, @t-will-gillis. It was very helpful. I went ahead and made the requested changes, which I'm testing now.

Copy link
Member

@t-will-gillis t-will-gillis left a comment

Choose a reason for hiding this comment

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

Hey @anthonypz Great work! I tested for the three different files scenarios, and the appropriate PR review message is posting on the issue. It looks like you were able to get a testing environment set up on your repo also. Looks good and thanks for making the extra revisions.

This is ready to merge, but I have one more request. It looks like in create-instructions.js lines 32, 50, 52, and 53 the indent from the original file is unaligned (tabs used instead of spaces?) Would you please fix this?

Great job again!

@anthonypz
Copy link
Member Author

anthonypz commented Apr 28, 2024

@t-will-gillis That's strange, VS Code usually converts tabs to spaces for me. Not sure what happened there.

I've just pushed a fix. Please let me know if anything else is needed.

@t-will-gillis t-will-gillis self-requested a review April 28, 2024 03:53
Copy link
Member

@t-will-gillis t-will-gillis left a comment

Choose a reason for hiding this comment

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

I am not even sure how to tab in VSCode because it converts for me also so maybe someone earlier was using a different editor?

Perfect- thanks again

Project Board automation moved this from test-pending-approval (Automated Column, do not place items here manually) to test-approved-by-reviewer (Automated Column, do not place items here manually) Apr 28, 2024
@t-will-gillis t-will-gillis merged commit 4295425 into hackforla:gh-pages Apr 28, 2024
3 checks passed
@HackforLABot HackforLABot removed this from test-approved-by-reviewer (Automated Column, do not place items here manually) in Project Board Apr 28, 2024
@anthonypz anthonypz deleted the custom-pr-instructions-5182 branch April 28, 2024 05:49
@t-will-gillis
Copy link
Member

Hey @anthonypz We had to revert this PR because the workflow had an error. I left a message for you on Slack also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Large Feature: Board/GitHub Maintenance Project board maintenance that we have to do repeatedly role: back end/devOps Tasks for back-end developers size: 3pt Can be done in 13-18 hours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customize PR instructions based on files in the PR
3 participants