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

Simplify Pull Request Template #3

Merged
merged 10 commits into from
Aug 12, 2022
Merged

Simplify Pull Request Template #3

merged 10 commits into from
Aug 12, 2022

Conversation

mindyls
Copy link
Contributor

@mindyls mindyls commented Aug 12, 2022

Related Issue

fixes NRLMMD-GEOIPS/geoips#17

Testing Instructions

Once PR template is merged to main, open pull request in any NRLMMD-GEOIPS repo to view new automated template.
Updated template: https://github.com/NRLMMD-GEOIPS/.github/blob/mindyls-patch-1/.github/pull_request_template.md
review-template.md: https://github.com/NRLMMD-GEOIPS/.github/blob/mindyls-patch-1/.github/review-template.md

Summary

NRLMMD-GEOIPS/geoips#17: 2022-08-12, simplify PR template

Documentation Updates

  • pull_request_template.md
    • Move "Reviewer Checklist" to bottom of PR template
    • Remove <issue_id> and from PR titles
      • Since PR contents are used directly in squashed commit messages, shorten PR title
    • Remove large instruction blocks
    • Remove example formats from Reviewer Checklist (title and issue)

Output

This PR follows new template.

Reviewer Checklist

Ensure you are logged into GitHub

Confirm all requirements are met for pull request

  • Ensure appropriate / sufficient content
    • Title format: geoips#<issue_num> <modified_repo_name> - <Short description>
    • Included Issue: Link to related Issue included at the beginning of pull request
    • CHANGELOG updated: Functionality included in pull request is ALSO referenced in CHANGELOG.md
    • Demonstrated Testing: Proper testing / output was demonstrated for functionality updates
    • Included Outputs: Imagery is included in "Output" section for new or changed product outputs (for reports!)
    • Test Scripts/Outputs for New Functionality: If new functionality is included in PR,
      ensure related test scripts and test outputs were included.
  • Ensure all appropriate tags / attributes added along right-hand side of pull request
    • Label: Added appropriate descriptors
    • Projects: GeoIPS - All Repos and All Functionality
      • Other projects allowed as appropriate
  • Ensure Related Issue is finalized appropriately (follow link above)

Once all items in checklist have been confirmed:

  • Approve pull request
    • Click "Files changed" tab
    • Click green "Review changes" button
    • Select "Approve" option
    • Add message if desired
    • Click green "Submit review" button
    • Project status will automatically be updated in Project "GeoIPS - All Repos and All Functionality" when pull request is approved.

* Remove large instruction blocks
* Remove example formats (title and issue)
* Move Reviewer Checklist to the bottom (maybe move it into a comment before merge - since PR contents get added to squashed commit message)
NRLMMD-GEOIPS/geoips#17
@mindyls mindyls added the git workflow Updates that streamline and improve the git workflow process label Aug 12, 2022
@mindyls mindyls self-assigned this Aug 12, 2022
* Shorten PR titles
* Remove large instruction blocks
* Remove example formats from Reviewer Checklist
* Move Reviewer Checklist to bottom
NRLMMD-GEOIPS/geoips#17
* Add ## header to individual Issue updates
* Add date to individual issue headers
* Separate out 2022-08-01 and 2022-08-12 udpates
Note we now intend to SAVE the individual Issue ID Headers in the CHANGELOG, so ensure they
are formatted properly from the start.

We will just add a version release header line on version release, rather than rearranging / removing
the individual Issue ID Headers.
NRLMMD-GEOIPS/geoips#17
@jsolbrig
Copy link
Contributor

My two cents:

  • Can we update "Related Issue" to "Related Issues" so that multiple can be included when needed?
  • Should the instructions be enclosed in block comments rather than using brackets? Something like this?
    <!---
    Please add one of the following here:
    - A link to ticket with testing instructions
    - A note that no exhaustive testing is required (if you have sufficient output below to demonstrate success, and it will be fully tested with next release)
    - Full instructions for testing this change
    --->
    
    This results in the instructions showing up in the template, but not in the final result if they aren't deleted.
  • Maybe he checklist should be converted to be true checklists using - [ ] and we shouldn't allow this to pass until everything has been checked off?
    • I can look into an action to block merging until the check boxes have been clicked.

To improve formatting (and ensure instructions do not appear within rendered PR if they are not deleted), replace instructions included in brackets with comment-based instructions.
NRLMMD-GEOIPS/geoips#17
Adding template to auto-populate PR reviews, rather than including directly within PR content itself.
NRLMMD-GEOIPS/geoips#17
Simplify the Pull Request Template so it does not include reviewer information - place that in review-template.md file.
NRLMMD-GEOIPS/geoips#17
@mindyls
Copy link
Contributor Author

mindyls commented Aug 12, 2022

My two cents:

  • Can we update "Related Issue" to "Related Issues" so that multiple can be included when needed?

  • Should the instructions be enclosed in block comments rather than using brackets? Something like this?

    <!---
    Please add one of the following here:
    - A link to ticket with testing instructions
    - A note that no exhaustive testing is required (if you have sufficient output below to demonstrate success, and it will be fully tested with next release)
    - Full instructions for testing this change
    --->
    

    This results in the instructions showing up in the template, but not in the final result if they aren't deleted.

  • Maybe he checklist should be converted to be true checklists using - [ ] and we shouldn't allow this to pass until everything has been checked off?

    • I can look into an action to block merging until the check boxes have been clicked.

Update "Related Issue" to "Related Issues" and update the sentence to reflect that change.
Copy link
Contributor

@jsolbrig jsolbrig left a comment

Choose a reason for hiding this comment

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

I think this looks good. I made one small change to update "Related Issue" to be plural.

@jsolbrig jsolbrig merged commit aff86b2 into main Aug 12, 2022
@jsolbrig jsolbrig deleted the mindyls-patch-1 branch August 12, 2022 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git workflow Updates that streamline and improve the git workflow process
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Update Git Workflow for GitHub.com functionality
2 participants