Skip to content

feat: add option to set custom user#331

Closed
yeikel wants to merge 2 commits intodependabot:mainfrom
yeikel:custom/user
Closed

feat: add option to set custom user#331
yeikel wants to merge 2 commits intodependabot:mainfrom
yeikel:custom/user

Conversation

@yeikel
Copy link
Contributor

@yeikel yeikel commented Apr 6, 2023

Closes #317

@yeikel yeikel requested a review from a team as a code owner April 6, 2023 00:41
steps:
- name: Fetch Dependabot metadata
id: dependabot-metadata
if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }}
Copy link
Contributor Author

@yeikel yeikel Apr 6, 2023

Choose a reason for hiding this comment

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

As discussed in #317

This check would be enough to prevent invalid users.

I extended the existing logic, but we should consider if we really need to have that check within the action when we can stop the execution before it even runs using the GitHub Events

cc @jeffwidman

Copy link
Member

Choose a reason for hiding this comment

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

This should definitely go in the README regardless of how we end up changing the code internals itself!

jeffwidman
jeffwidman previously approved these changes Apr 6, 2023
@@ -21,7 +21,7 @@ export async function getMessage (client: InstanceType<typeof GitHub>, context:

// Don't bother hitting the API if the PR author isn't Dependabot
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the reason the check exists within the code... the concern is that users may not realize they need to set that, and so may fire off spurious requests... so it's defensive coding to protect the API from ignorant/careless users.

That said, I agree the volume of calls here is probably pretty low, so it may be fine to remove this... 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

I filed #332 to track this discussion as it's out of scope of this PR.

@jeffwidman jeffwidman enabled auto-merge (squash) April 6, 2023 20:59
auto-merge was automatically disabled April 7, 2023 16:04

Head branch was pushed to by a user without write access

@jeffwidman
Copy link
Member

See the suggestion in #317 (comment) for creating a second PR with an alternative approach... leaving this one open just in case i'm forgetting something. We'll merge one or the other.

@jeffwidman jeffwidman dismissed their stale review April 7, 2023 21:22

Considering a public API design change

@yeikel
Copy link
Contributor Author

yeikel commented Apr 12, 2023

@jeffwidman I created #336

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.

Add support for dependabot-script/custom commit users

2 participants