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 GHA workflows to auto-merge dependency updates and bundle GB dependency updates per GB plugin release #6302

Merged
merged 11 commits into from
Jun 2, 2021

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented May 21, 2021

Summary

Fixes #6298

Adds GHA workflows to:

  • Auto-merge PRs from Dependabot that are approvelisted
  • Check for Gutenberg plugin updates and if there is a new one create a PR with the set of Gutenberg related dependency updates

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

Comment on lines +13 to +15
if: >
# Auto merge `@wordpress/` dependency PRs
startsWith( ${{ github.event.pull_request.title }}, 'Bump @wordpress' )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can update this if condition as a means to whitelist auto-merging for other dependency updates.

Copy link
Member

Choose a reason for hiding this comment

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

At the moment this won't do anything because the @wordpress dependencies are excluded, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, they're ignored in the Dependabot config.

@pierlon pierlon marked this pull request as ready for review May 21, 2021 04:13
@pierlon pierlon requested review from westonruter and schlessera May 21, 2021 04:16
@pierlon pierlon self-assigned this May 21, 2021
@pierlon pierlon added the Infrastructure Changes impacting testing infrastructure or build tooling label May 21, 2021
@pierlon pierlon added this to the v2.2 milestone May 21, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 21, 2021

Plugin builds for aadb449 are ready 🛎️!

@pierlon
Copy link
Contributor Author

pierlon commented May 22, 2021

In 4e54f17 I added a new GHA workflow that monitors the Gutenberg repo for plugin releases, and if there is one it opens a PR containing the related dependency updates since that release.

@pierlon pierlon changed the title Add GHA workflow to auto-merge some dependency updates Add GHA workflows to auto-merge dependency updates and bundle GB dependency updates per GB plugin release May 22, 2021
echo "::set-output name=version::$(echo "$LAST_VERSION")"
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
QUERY: 'repo:ampproject/amp-wp is:pr is:open is:merged in:title Update Gutenberg packages after'
Copy link
Member

Choose a reason for hiding this comment

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

Should this do an author check as well to be safe?

Copy link
Contributor Author

@pierlon pierlon May 25, 2021

Choose a reason for hiding this comment

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

Done in 46b6ef7.

id: last-release
run: |
PR_TITLE=$(gh api -X GET search/issues -f q='${{ env.QUERY }}' -f sort='created' -f order='desc' --jq '.items.[].title' | head -n 1)
LAST_VERSION=$(node -e "'$PR_TITLE' && console.log( '$PR_TITLE'.match('v(.+) release')[1] )")
Copy link
Member

Choose a reason for hiding this comment

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

What's the '$PR_TITLE' && part for?

If the matched pull request contains any quotation marks (perhaps by malicious user), then this will cause an error.

Perhaps something like this would be better:

Suggested change
LAST_VERSION=$(node -e "'$PR_TITLE' && console.log( '$PR_TITLE'.match('v(.+) release')[1] )")
LAST_VERSION=$(sed 's/.* \(v.*\).*/$1/' <<< "$PR_TITLE")

Copy link
Member

Choose a reason for hiding this comment

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

This could then also do a check to see if it the resulting LAST_VERSION matches ^v\d+(\.\d+)*$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f0a0dab.


- name: Determine if with package updates are needed
id: release-status
run: echo "::set-output name=outdated::$(php -r "echo json_encode(version_compare('$LATEST_VER', '$LAST_VER', '>'));")"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
run: echo "::set-output name=outdated::$(php -r "echo json_encode(version_compare('$LATEST_VER', '$LAST_VER', '>'));")"
run: echo "::set-output name=outdated::$(php -r "echo json_encode(version_compare($argv[1], $argv[2], '>'));" "$LATEST_VER" "$LAST_VER" )"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Close! I had to instead wrap the command in single quotes to get it working.

Copy link
Contributor Author

@pierlon pierlon May 25, 2021

Choose a reason for hiding this comment

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

Done in 52f3426.

@pierlon pierlon force-pushed the enhancement/6298-automerge-dependency-prs branch from fcb0065 to f0a0dab Compare May 25, 2021 09:35
@pierlon pierlon requested a review from westonruter May 25, 2021 09:37
@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #6302 (f0a0dab) into develop (3bb15f3) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head f0a0dab differs from pull request most recent head aadb449. Consider uploading reports for the commit aadb449 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #6302   +/-   ##
==========================================
  Coverage      75.39%   75.40%           
  Complexity      5881     5881           
==========================================
  Files            235      235           
  Lines          17787    17781    -6     
==========================================
- Hits           13411    13407    -4     
+ Misses          4376     4374    -2     
Flag Coverage Δ
javascript 79.84% <ø> (ø)
php 75.20% <ø> (+<0.01%) ⬆️
unit 75.20% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
includes/class-amp-theme-support.php 86.05% <0.00%> (-0.02%) ⬇️
includes/amp-helper-functions.php 81.89% <0.00%> (+0.19%) ⬆️

@@ -25,18 +25,18 @@ jobs:
id: last-release
run: |
PR_TITLE=$(gh api -X GET search/issues -f q='${{ env.QUERY }}' -f sort='created' -f order='desc' --jq '.items.[].title' | head -n 1)
LAST_VERSION=$(node -e "'$PR_TITLE' && console.log( '$PR_TITLE'.match('v(.+) release')[1] )")
LAST_VERSION=$(sed -r 's/.+ v(.+) .+/\1/' <<< "$PR_TITLE")
echo "::set-output name=version::$(echo "$LAST_VERSION")"
Copy link
Member

Choose a reason for hiding this comment

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

In order to ensure only valid version numbers are used, this could could be checked as follows:

Suggested change
echo "::set-output name=version::$(echo "$LAST_VERSION")"
if egrep -q '^[0-9][0-9]*(\.[0-9][0-9]*)*$' <<< "$LAST_VERSION"; then
echo "::set-output name=version::$(echo "$LAST_VERSION")"
fi

Copy link
Member

Choose a reason for hiding this comment

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

And maybe exit 1 if else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not a non-zero exit, as that would mark the job as a failure which is techincally not true... unless that's what we want?

Copy link
Member

@westonruter westonruter Jun 2, 2021

Choose a reason for hiding this comment

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

At least the job should be skipped entirely?

Er, but at the moment there are no such merged PRs in the repo: https://github.com/ampproject/amp-wp/pulls?q=is%3Apr+author%3Aapp%2Fgithub-actions+is%3Aopen+is%3Amerged+in%3Atitle+Update+Gutenberg+packages+after

I guess in that case LAST_VER will be empty? So then maybe it should be set to 0.0.0:

Suggested change
echo "::set-output name=version::$(echo "$LAST_VERSION")"
if ! egrep -q '^[0-9][0-9]*(\.[0-9][0-9]*)*$' <<< "$LAST_VERSION"; then
LAST_VERSION='0.0.0'
fi
echo "::set-output name=version::$(echo "$LAST_VERSION")"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works 👍.

Comment on lines +13 to +15
if: >
# Auto merge `@wordpress/` dependency PRs
startsWith( ${{ github.event.pull_request.title }}, 'Bump @wordpress' )
Copy link
Member

Choose a reason for hiding this comment

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

At the moment this won't do anything because the @wordpress dependencies are excluded, right?

if: steps.packages.outputs.list != 0 && steps.remote-branch.outputs.exists == 0
run: |
git push -u origin "$HEAD_BRANCH"
gh pr create --base "$BASE_BRANCH" --title "Update Gutenberg packages after v$VERSION release" --body "" --label dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Should this also do gh pr merge --auto --merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that works here as well, will make the change.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM

@westonruter westonruter merged commit c030292 into develop Jun 2, 2021
@westonruter westonruter deleted the enhancement/6298-automerge-dependency-prs branch June 2, 2021 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GitHub action to auto-merge Dependabot pull requests
2 participants