Skip to content

Check PR Author instead of Action Actor#137

Merged
brrygrdn merged 2 commits intodependabot:mainfrom
pangaeatech:use_author
Feb 21, 2022
Merged

Check PR Author instead of Action Actor#137
brrygrdn merged 2 commits intodependabot:mainfrom
pangaeatech:use_author

Conversation

@mwaddell
Copy link
Copy Markdown
Contributor

@mwaddell mwaddell commented Feb 7, 2022

closes #112

@mwaddell mwaddell requested a review from a team as a code owner February 7, 2022 17:25
@mwaddell mwaddell marked this pull request as draft February 12, 2022 02:36
@mwaddell mwaddell marked this pull request as ready for review February 12, 2022 02:36
dependabot:
runs-on: ubuntu-latest
if: ${{ github.actor == 'dependabot[bot]' }}
if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✨ Thanks for updating the documentation as well

Copy link
Copy Markdown
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

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

Thanks @mwaddell! This change looks good to me.

@brrygrdn brrygrdn merged commit 4aac239 into dependabot:main Feb 21, 2022
@brrygrdn brrygrdn mentioned this pull request Feb 21, 2022
@xt0rted
Copy link
Copy Markdown

xt0rted commented Feb 25, 2022

I adjusted my workflow to take advantage of this change, but now it fails if I add commits to to the PR.

image

Typical reasons for this would be to add an entry to the changelog, fix linting rules, or as happened today fix an incorrect version update (a couple actions used v2 instead of v2.0.0).

What's the best way to work around this? Maybe a new output indicating if non-dependabot commits were found so you could skip subsequent steps?

@mwaddell
Copy link
Copy Markdown
Contributor Author

@brrygrdn What's the reason for failing the run if the PR contains commits after the original dependabot one? Is there a good reason not to delete https://github.com/dependabot/fetch-metadata/blob/main/src/dependabot/verified_commits.ts#L35:L38

@mwaddell
Copy link
Copy Markdown
Contributor Author

@brrygrdn What's the reason for failing the run if the PR contains commits after the original dependabot one? Is there a good reason not to delete https://github.com/dependabot/fetch-metadata/blob/main/src/dependabot/verified_commits.ts#L35:L38

@brrygrdn - I added #166 to remove those lines in order to address the issue noted by @xt0rted - let me know if there's a reason we can't do this.

@brrygrdn
Copy link
Copy Markdown
Contributor

brrygrdn commented Feb 26, 2022

That's a good question, it's something I've actually been blocked by myself this week. I originally added it as we were (defensively) trying to facilitate narrowest possible definition of a Dependabot PR, i.e.

  • Created by Dependabot
  • Exclusively containing code committed by Dependabot

I don't think any potential benefit or risk it mitigates really offsets the fact that an extremely common workflow like merging in the target branch is blocked. I'll have a chat with the team on Monday morning to get a second opinion, but I'm leaning towards removing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Request: Check PR Author Instead of Action Actor

3 participants