Skip to content

devtools: add pr-tool to automate PR review and merge#935

Merged
pstorz merged 27 commits intobareos:masterfrom
arogge:dev/arogge/master/changelog-bot
Jan 20, 2023
Merged

devtools: add pr-tool to automate PR review and merge#935
pstorz merged 27 commits intobareos:masterfrom
arogge:dev/arogge/master/changelog-bot

Conversation

@arogge
Copy link
Member

@arogge arogge commented Sep 24, 2021

Add a new tool pr-tool to the set of existing devtools. The tool supports differents modes of operation.
When invoked with pr-tool check it will run some sanity checks for the PR itself and look for some of the things the reviewer had to check manually before:

  • see that changes are approved
  • make sure all checkmarks have been set
  • make sure the integration build finished successfully
  • ensure formatting restrictions of commits
  • warn if there are commits with certain keywords ("fixup" or anything starting with "[...]")
  • make sure there is a ChangeLog entry (or make sure one can be added automatically)

With pr-tool merge the above checks will be run and if these are successful, a ChangeLog entry will be added if required (including a rebase onto the base-branch if otherwise there is a conflict) and then the PR will be merged.

Finally pr-tool add-changelog will just add the ChangeLog record and commit CHANGELOG.md (without pushing).

TODO

  • run commit checks on local commits
  • make sure adding a changelog entry doesn't break an existing multiline entry
  • provide a way to add a changelog record in the "Documentation" section
  • maybe allow PR labels to select changelog section
  • display to which section the changelog record would be added (any maybe even why)
  • ensure base-branch is up-to-date before rebasing
  • run bareos-check-sources on the relevant commit-range

Thank you for contributing to the Bareos Project!

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General
  • PR name is meaningful
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR
  • bareos-check-sources --since-merge does not report any problems
  • git status should not report modifications in the source tree after building and testing

@arogge arogge added the nobuild label Sep 24, 2021
@arogge
Copy link
Member Author

arogge commented Sep 24, 2021

do you think we will need to test this on the bareos-testing project before merging it here?

@arogge arogge force-pushed the dev/arogge/master/changelog-bot branch from 56247d8 to 51a7fd5 Compare September 30, 2021 16:00
@arogge arogge marked this pull request as draft October 6, 2021 13:10
@arogge
Copy link
Member Author

arogge commented Oct 6, 2021

This cannot work right now, as we will have to adapt github permissions and branch protection to match our needs.

@arogge arogge force-pushed the dev/arogge/master/changelog-bot branch 7 times, most recently from 6b2ec9d to e85ceaf Compare December 1, 2022 14:46
@arogge arogge changed the title Add action for automated changelog updates Add developer-tool to support the pull-request review and merge process Dec 7, 2022
@arogge arogge force-pushed the dev/arogge/master/changelog-bot branch from dcd1f87 to 5858302 Compare December 7, 2022 18:05
@arogge arogge force-pushed the dev/arogge/master/changelog-bot branch 3 times, most recently from 31f944a to 04fca20 Compare December 23, 2022 12:09
@arogge
Copy link
Member Author

arogge commented Dec 23, 2022

I think it is now pretty much finished.

There are some considerations we may discuss next year:

  • maybe we can run bareos-check-sources and the gh commands in parallel to save more time
  • maybe we need to rerun bareos-check-sources --all --modify on the maintained branches again, as some of the tools seem to format slightly different now

@arogge arogge marked this pull request as ready for review December 23, 2022 12:16
@arogge arogge force-pushed the dev/arogge/master/changelog-bot branch from eb83e56 to c8637cc Compare January 9, 2023 13:27
@arogge arogge force-pushed the dev/arogge/master/changelog-bot branch 2 times, most recently from d505964 to 42e1eec Compare January 12, 2023 17:49
Copy link
Member

@pstorz pstorz left a comment

Choose a reason for hiding this comment

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

Good work!

@pstorz pstorz changed the title Add developer-tool to support the pull-request review and merge process devtools: add pr-tool to automate and support the PR review and merge process Jan 20, 2023
@pstorz pstorz changed the title devtools: add pr-tool to automate and support the PR review and merge process devtools: add pr-tool to automate PR review and merge Jan 20, 2023
@pstorz pstorz changed the title devtools: add pr-tool to automate PR review and merge devtools: add pr-tool to automate PR review and merge Jan 20, 2023
arogge and others added 26 commits January 20, 2023 11:24
Introduce a new devtool "pr-tool" that helps merging pull requests. It
can check prerequisites, add CHANGELOG.md entries and automatically
rebase to avoid CHANGELOG.md merge conflicts.

Also add a python version of update-changelog-links.sh that is exposed
as a program and used internally by pr-tool.
* Check that PR status is OPEN and not CLOSED or MERGED.
* Display url of PR
* Add parameter --admin-override to merge. Will call the 'gh pr merge'
  command with the --admin option to override required checks if
  possible
* Look for github status checks and stop merge if not successful.
  Provide option --ignore-status-checks in merge to skip this check.
rerunning "pipenv lock" to get newer versions of everything we're using.
pr-tool will now also check if the status check with context
"continuous-integration/jenkins/pr-merge" is present and fail the PR
check otherwise.
"pr-tool merge" will now show if PR was merged or not before exiting.
The headline-check on pr-tool commit now allows up to 60 characters.
Enables pr-tool check to display the section to which the changelog
record will be added.
The pr-tool is now able to select the section to which a ChangeLog entry
should be added based on the PR's labels.
Currently this will only check for the "documentation" label and set the
section to Documentation (which was not possible before).
add_changelog_entry() did a bad job if there were existing multiline
entries in the changelog. If the section where a new entry should be
added ended in something like this:

- normal changelog entry
  - with additional information
  - and more information

it would misdetect the location to add the new record and insert it
after the first line instead of at the end.
Now it also skips lines where "- " is preceded by whitespace.
You can now pass absolute paths as a parameter to --file when running
add-changelog-entry.
The original add-changelog-entry would always interpret its --file
argument relatively to the current git repository, even if the path was
absolute.
The user can override the host or repository for the gh command using
the environment variables GH_HOST or GH_REPO. As we will have to
determine the repository and the name of the remote ourselves (gh does
not provide a way to do that), setting GH_HOST or GH_REPO will probably
break pr-tool at some point, so we disallow it.

Also introduces logging options.
For doing a rebase, we need to know the (remote) branch we're rebasing
onto. While this is usually "origin" you can call it whatever you want.
We now determine the correct git remote in the same fashion as gh cli
does.
When your local repository is not up to date, rebasing (and potentially
other operations) will not lead to the desired result or may even fail.
pr-tool will now check if your local repository's base branch points to
the same commit as the remote repository's base branch and will issue a
warning otherwise.
When checking commits, pr-tool will now look at the local history
instead of the commits in the PR at github. So you can reword and
recheck without pushing.
With this patch, you can now invoke check_sources.main_program() from
your own code and get meaningful results.
When running pr-tool check or pr-tool merge, an automated run of
bareos-check-sources --since=<your-merge-base-commit> happens and its
result is evaluated as a checkmark result for the PR.

As this is now automated, we can remove the checkmark from the PR
template.
By using another approach to detect changed files per commit, the
performance of bareos-check-sources could be drastically improved.
Instead of looking at the mime-type, we now simply check if the file is
empty and try to read it as utf-8 otherwise. This turned out to be
faster than guessing the mime-type first.
We can probably improve this further by adding some filename patterns
that should always be skipped (like *.gz, *.tgz, *.png, *.min.*).
This plugin shrinks block comments by merging alone standing
begin and end of block comments (/* and */) into the second and in the
last line, respectively.

Block comments starting in the first column remain untouched, as these
are usually file license information or function documentation.
Ran rm Pipfile.lock && pipenv install
@pstorz pstorz force-pushed the dev/arogge/master/changelog-bot branch from d5c640c to b0c0445 Compare January 20, 2023 10:24
@pstorz pstorz merged commit 694488c into bareos:master Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants