Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions .github/workflows/cron-update-rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ jobs:
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
- uses: tibdex/github-app-token@v1
id: generate-token
with:
app_id: ${{ secrets.APP_ID }}
private_key: ${{ secrets.APP_PRIVATE_KEY }}
Comment on lines +13 to +17
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need this new GH Action ?

Although it's suggested in the peter-evans/create-pull-request, can't we just follow the same approach as rust-bitcoin (here) and use a new token ?

- uses: crazy-max/ghaction-import-gpg@v5
with:
gpg_private_key: ${{ secrets.GPG_PRIVATE_KEY }}
git_user_signingkey: true
git_commit_gpgsign: true
Comment on lines +18 to +22
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm unsure how this action impact security-wise, but it doesn't seem very used/maintained :think:. We can always sign off the commits if we opt to not use this action.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It looks like the rust-bitcoin repo doesn't enforce that the commits be signed. With our current repo rules we can't merge unsigned commits. https://github.com/rust-bitcoin/rust-bitcoin/pull/3267/commits

So we'd need to take off this protection, or figure out a way to get the github action to create a properly signed commit.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks like it will work, but I want to make sure we've considered the alternatives and that these new actions won't be a hassle to maintain for someone like @notmandatory

Options:

  • for security, fork or pin third-party actions to a commit SHA (requires code review)
  • drop requirement to sign commits (not ideal)
  • allow actions to create PR but don't merge it (not ideal)
  • just pin clippy to stable and forgo the extra automation

Copy link
Copy Markdown
Member Author

@notmandatory notmandatory Aug 29, 2024

Choose a reason for hiding this comment

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

I'm OK with these automations since they will prevent us from getting unexpected breaks when stable updates behind the scenes. My main concern is I don't want to use my (or anyone else's) github or gpg credentials to create and sign the PR commit.

I think the github app credentials for PR creation as I have it now are a fine way to go. If the GPG plugin I added doesn't work then we could just let the action create the PR and who ever merges it will need to first rebase-resign the commit manually. It shouldn't happen too often so I'm not worried about the extra step.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

BTW I think I can test this in my personal repo, will post here to confirm if it works as expected.

Also I'm not so worried about the plugin versions being pinned since this can only create PRs it can't commit them.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I tested on my local repo and it works, see: https://github.com/notmandatory/bdk/pull/9/commits

I had to create a new GPG key for the action, add the corresponding GPG pubkey to my account, and finally verify the email for this GPG key with my account. But after all that github created the PR, automatically runs the CI, and is able to verify the commit so the PR can be merged. 🎉

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I tested on my local repo and it works, see: https://github.com/notmandatory/bdk/pull/9/commits

Do you happen to know why the ci ran 41 checks on that PR?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess it's because it ran on pull_request and push dispatches 🤔

- name: Update rust-version to use latest stable
run: |
set -x
Expand All @@ -30,9 +40,9 @@ jobs:
if: env.changes_made == 'true'
uses: peter-evans/create-pull-request@v6
with:
token: ${{ secrets.GITHUB_TOKEN }}
author: Update Rustc Bot <bot@example.com>
committer: Update Rustc Bot <bot@example.com>
token: ${{ steps.generate-token.outputs.token }}
author: Github Action <github@bitcoindevkit.org>
committer: Github Action <github@bitcoindevkit.org>
branch: create-pull-request/update-rust-version
title: |
ci: automated update to rustc ${{ env.rust_version }}
Expand Down