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

Allow custom dependabotAuthor and dependabotCommitter #378

Closed
2 tasks done
yeikel opened this issue Feb 28, 2023 · 19 comments · Fixed by #411
Closed
2 tasks done

Allow custom dependabotAuthor and dependabotCommitter #378

yeikel opened this issue Feb 28, 2023 · 19 comments · Fixed by #411
Assignees

Comments

@yeikel
Copy link
Contributor

yeikel commented Feb 28, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

Add two new configuration properties dependabotAuthor and dependabotCommitter that allows users to configure https://github.com/fastify/github-action-merge-dependabot/blob/main/src/getDependabotDetails.js#L2-L4 and set custom values

note : I am open to sending a PR about this

Motivation

I run dependabot-core (the core framework behind dependabot) privatel yand my user details are not what the actions expect

Example Configuration

dependabotAuthor:
   required: false
   default: "dependabot[bot]"
dependabotCommitter:
   required: false
   default: "GitHub"

Example Input

dependabotAuthor : myUser
@yeikel yeikel changed the title Allow custom getDependabotDetails Allow custom dependabotAuthor and dependabotCommitter Feb 28, 2023
@climba03003
Copy link
Member

The first question is, does https://github.com/dependabot/fetch-metadata support the same input?
If not, I am pretty much against it since there are no way for us to retrieve the commit info.

@climba03003
Copy link
Member

climba03003 commented Feb 28, 2023

@yeikel
Copy link
Contributor Author

yeikel commented Feb 28, 2023

Thank you for pointing that out. I started a discussion in that project first dependabot/fetch-metadata#317 and I'll report back

@yeikel
Copy link
Contributor Author

yeikel commented Apr 18, 2023

It seems like it is not supported by dependabot/fetch-metadata either.

https://github.com/dependabot/fetch-metadata/blob/29dc6db06eafcbc87b2ecc3b760911cc60b43423/src/dependabot/verified_commits.ts#L22-L44

We implemented this slightly different in fetch-metadata. We simply skip all the checks

https://github.com/dependabot/fetch-metadata/blob/main/action.yml#L20-L23

Would you be open to adding something similar now?

@climba03003
Copy link
Member

Would you be open to adding something similar now?

Skipping all the verification would be a dangerous action.
I am OK with an always showing warning.
cc @simoneb Just ping for advice.

@yeikel
Copy link
Contributor Author

yeikel commented Apr 18, 2023

Would you be open to adding something similar now?

Skipping all the verification would be a dangerous action.
I am OK with an always showing warning.
cc @simoneb Just ping for advice.

When I say "skip verification" I am referring to the dependabot verifications. What I do need is to skip the author verification as my pull requests are coming from a service id

@simoneb
Copy link
Collaborator

simoneb commented Apr 18, 2023

If the fetch-metadata action accepted this change, I think we can do the same here. I'm just not sure that skip-verification makes a lot of sense in our context, so I'd aim for something less generic and more specific to what that setting is actually doing. Any ideas what this setting may look like @yeikel ? Btw thanks for staying on top of this and contributing both here and the dependabot action!

@yeikel
Copy link
Contributor Author

yeikel commented Apr 18, 2023

If the fetch-metadata action accepted this change, I think we can do the same here. I'm just not sure that skip-verification makes a lot of sense in our context, so I'd aim for something less generic and more specific to what that setting is actually doing. Any ideas what this setting may look like @yeikel ? Btw thanks for staying on top of this and contributing both here and the dependabot action!

How about skip-author-verification?

The author can be enforced using Github actions directly, and we can let the user decide how that looks under their environment. For all we know, it could be one or multiple users

If it helps, one more scenario that will be more common until dependabot supports grouping is when one or more dependabot commits are merged into a single larger commit. This can happen manually or even automatically. See https://github.com/hrvey/combine-prs-workflow

@simoneb
Copy link
Collaborator

simoneb commented Apr 18, 2023

If the fetch-metadata action accepted this change, I think we can do the same here. I'm just not sure that skip-verification makes a lot of sense in our context, so I'd aim for something less generic and more specific to what that setting is actually doing. Any ideas what this setting may look like @yeikel ? Btw thanks for staying on top of this and contributing both here and the dependabot action!

How about skip-author-verification?

Works for me, do you think this will play well with the existing skip-commit-verification?

The author can be enforced using Github actions directly, and we can let the user decide how that looks under their environment. For all we know, it could be one or multiple users

I'm not sure what you mean by this, can you clarify?

If it helps, one more scenario that will be more common until dependabot supports grouping is when one or more dependabot commits are merged into a single larger commit. This can happen manually or even automatically. See https://github.com/hrvey/combine-prs-workflow

I'm also not sure I understand what you mean with this and how this is relevant for this issue.

@yeikel
Copy link
Contributor Author

yeikel commented Apr 18, 2023

Works for me, do you think this will play well with the existing skip-commit-verification?

Honestly, they would not be compatible as skip-commit-verification will always fail in that scenario. That's why we implemented it as skip-verification in fetch-metadata as both the user and the commit signature will be different

I'm not sure what you mean by this, can you clarify?

In Github Actions, we can add a check like this:

- name: Greet dependabot
   if: ${{ github.event.pull_request.user.login == 'dependabot[bot]' }} / #This step will only run if the user matches. The "echo" command won't run at all
  shell: echo "Hello dependabot!"

As this feature exists, the only reason to have this additional verification in the action code is for safety, but normally I would implement it using the native Github Actions feature instead to avoid the call altogether

I'm also not sure I understand what you mean with this and how this is relevant for this issue.

Apologies that it was not clear. What I meant is that there are other scenarios where this feature would be useful that I discovered recently. These "bundled" pull requests are created by other users and the commit verification will also fail

@simoneb
Copy link
Collaborator

simoneb commented Apr 18, 2023

Thanks for the clarifications @yeikel. On the first point, how do you suggest to progress? We can certainly consider removing the existing "skip" input, but clearly if we have a non-breaking way to do this it would be even better.

@yeikel
Copy link
Contributor Author

yeikel commented Apr 18, 2023

I think that the easiest way to proceed is to add a new parameter skip-author-verification and if set, we should document that skip-commit-verification is also disabled.

The other option is to implement it just like fetch-metadata did so that when set, both the author and the commit verification are skipped

As a side note, I just noticed that in your action definition, you're also filtering by author and that we would need to enhance that as well using the same parameter

https://github.com/fastify/github-action-merge-dependabot/blob/main/action.yml#L51

Is the check in the code necessary given that? I am not sure in what situations the action is running for other users when we are stopping it from running using that check

@simoneb
Copy link
Collaborator

simoneb commented Apr 20, 2023

I am happy to go with this solution then, yes!

And yes we'll have to change some conditions in the action yaml definition as well.

@yeikel I would appreciate if you could send a PR for this 🙏

@yeikel
Copy link
Contributor Author

yeikel commented Apr 20, 2023

I am happy to go with this solution then, yes!

And yes we'll have to change some conditions in the action yaml definition as well.

@yeikel I would appreciate if you could send a PR for this 🙏

Can you clarify what option are we settling for?

  1. Add a flag skip-verification that, just like fetch metadata, skips the author and commits verification if enabled

  2. Add a field to customize the user that can act as dependabot. If enabled, commit verification should be disabled

(I personally prefer #1 to keep it consistent with fetch-metadata)

@simoneb
Copy link
Collaborator

simoneb commented Apr 20, 2023

I'd go with #1 yeah

@yeikel
Copy link
Contributor Author

yeikel commented Apr 20, 2023

I'd go with #1 yeah

Great, thank you for accepting the request!

I'll work on it this week and feel free to assign the issue to me :)

@simoneb
Copy link
Collaborator

simoneb commented Apr 20, 2023

Deal, thanks a lot 👌

yeikel added a commit to yeikel/github-action-merge-dependabot that referenced this issue Apr 24, 2023
When enabled, both author and commit verification are disabled

Closes fastify#378
yeikel added a commit to yeikel/github-action-merge-dependabot that referenced this issue Apr 24, 2023
When enabled, both author and commit verification are disabled

Closes fastify#378
@yeikel
Copy link
Contributor Author

yeikel commented Apr 24, 2023

Deal, thanks a lot 👌

I submitted #411 as promised

Thank you!

simoneb pushed a commit that referenced this issue Apr 25, 2023
* feat: add option to skip-verification

When enabled, both author and commit verification are disabled

Closes #378

* Update src/action.js

Co-authored-by: KaKa <climba03003@gmail.com>

* Update src/action.js

Co-authored-by: KaKa <climba03003@gmail.com>

* regenerate dist

* fix linting

---------

Co-authored-by: KaKa <climba03003@gmail.com>
@github-actions
Copy link

github-actions bot commented May 3, 2023

🎉 This issue has been resolved in version 3.7.0 🎉

The release is available on:

Your optic bot 📦🚀

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 a pull request may close this issue.

3 participants