search

BobAnkh / add-contributors

A Github Action to add contributors to your file automatically. Feel free to contribute!
https://github.com/marketplace/actions/auto-add-contributors
Apache License 2.0
33 stars 20 forks source link

[enhancement] Pull request instead of direct commit #49

Closed casperklein closed 3 years ago

casperklein commented 3 years ago

Is your feature request related to a problem? Please describe

When having a protected branch, you cannot directly commit to that. There must be a PR with approval of several people, checks passed etc (depends on how it is setup).

Description of the new feature/enhancement

Instead of directly committing to the branch, a PR should be opened (but only when there are changes, not every time `add-contributors is run on schedule).

boring-cyborg[bot] commented 3 years ago

Thanks for opening your first issue here! Be sure to follow the issue template!

gitmagic-app[bot] commented 3 years ago

Everything looks good :thumbsup:

gitmagic-app[bot] commented 3 years ago

Everything looks good :thumbsup:

BobAnkh commented 3 years ago

Great idea!

So do you want to send me a PR on this feature @casperklein? Or I will do it next week, as I'm busy this week.

BobAnkh commented 3 years ago

Besides, do you think an additional param like PULL_REQUEST is good for use? You can specify the PR from head branch to base branch in format <head>:<base>. A PR will only be opened when file is updated.

casperklein commented 3 years ago

Unfortunately, I cannot provide a PR (I am totally unfamiliar with python). I understand, that a FR can take some time to get implement or is not implemented at all 👍

I see two options to implement this:

  1. PULL_REQUEST as boolean true/false (default: false, to not break existing setups)
  2. COMMIT_MODE with values of (commit|pr). Default: commit
BobAnkh commented 3 years ago

Ok, thank you for your suggestions on the FR, I will consider them next week to see how to implement this feature

BobAnkh commented 3 years ago

Hi, because you can use format <branch>:<file> in the parameter PATH to specify which branch to push your changes(I will make the reference more clearly), so I would like to implement this FR like this:

How do you like this idea? @casperklein

casperklein commented 3 years ago

TBH I find this a bit confusing at first. I would stick with "one parameter - one option":

  1. PULL_REQUEST: true/false
  2. PULL_REQUEST_TARGET: target branch

That said, the same goes for the current PATH parameter. I would equalize it:

  1. PATH Path to the file you want to add contributors' list
  2. BRANCH Specify the branch of the file, defined in path. (optional)

This approach would make the "PATH note" below the parameter table in the README obsolete - which will surely be overlooked by some (including me until now 😄). If you want to keep it as it is, I suggest adding the note text to the PATH description in the table.

In the end, it's up to you. Whatever works, is good 👍

BobAnkh commented 3 years ago

Really good suggestions. I will break the original PATH into PATH and BRANCH. But to the PULL_REQUEST feature, I still decide to add one param PULL_REQUEST. If you want to open a pr, then set PULL_REQUEST to the target branch, and if you don't want to open a pr, simply skip the param.

Anyway, thank you for your suggestions.

BobAnkh commented 3 years ago

@casperklein

Hi, I have added the new feature, so could you please test it to see if it satisfies your opinion?

casperklein commented 3 years ago

For sure. Can you advice me, how to do so?

uses: BobAnkh/add-contributors@v0.2.1

Is there a way, to use the "master" branch of your repo as action instead of the version?

BobAnkh commented 3 years ago

I recommend you to use the format in the workflow in this repo temporarily to use the master branch:

uses: BobAnkh/add-contributors@master

And after tested, I will release a new version, so that you can use the new version then.

casperklein commented 3 years ago 
main()
  File "/main.py", line 318, in main
    Contributors.write_data(content)
  File "/main.py", line 115, in write_data
    self.repo.create_pull(title=self.COMMIT_MESSAGE, base=self.PULL_REQUEST, head=self.BRANCH)
  File "/usr/local/lib/python3.9/site-packages/github/Repository.py", line 1350, in create_pull
    return self.__create_pull_2(*args, **kwds)
TypeError: __create_pull_2() got an unexpected keyword argument 'title'

My test repo: https://github.com/barthheiko/docker-mailserver/runs/3698589721?check_suite_focus=true#step:4:239

BobAnkh commented 3 years ago

Ok, I see, I will fix the bug later. Also please remember to set param BRANCH together with PULL_REQUEST. Only with both params given will a new pr opened. (e.g. you cannot open a pr from master to master)

Many thanks for help.

casperklein commented 3 years ago

you cannot open a pr from master to master

Oh, that's bad (but makes sense 😄). I want to update the master branch, but also need a PR because this branch is protected.

Could your action create a temporary branch and then create a PR from that? I am wondering, how "Dependabot" works. This action also creates a PR against the master branch.

Edit: Or I create a temporary branch myself, a step before running your action. This should work?

BobAnkh commented 3 years ago

Edit: Or I create a temporary branch myself, a step before running your action. This should work?

This definitely will work.

I am using the api here to update file and here to create a new pr, not sure if it will create a new branch if not exists. Maybe you can test it when I fix the bug.

BobAnkh commented 3 years ago

Actually I can do the whole process as the dependabot does, creating a pr, auto-merging it and closing the pr, deleting the branch, etc. But, I suppose this is out of the scope of this action.

BobAnkh commented 3 years ago

Hey, I think this bug has been removed in the recent commit.

(Seems like a kind of weird api design in pygithub)

casperklein commented 3 years ago

TypeError: __create_pull_1() missing 1 required positional argument: 'body'

https://github.com/barthheiko/docker-mailserver/runs/3699658794?check_suite_focus=true#step:5:295

casperklein commented 3 years ago

I tried to fix it myself: https://github.com/casperklein/add-contributors/commit/d7225406115c1941122f77aded60d5de4bfb2ea4

But now rate-limit kicks in 😠 https://github.com/barthheiko/docker-mailserver/runs/3699790935?check_suite_focus=true#step:5:136

I will try again later.

casperklein commented 3 years ago

It works, thank you!

Action: https://github.com/barthheiko/docker-mailserver/runs/3700809790?check_suite_focus=true PR: https://github.com/barthheiko/docker-mailserver/pull/1

BobAnkh commented 3 years ago

Really thank you for your help, sorry for missing that param 🙏

You are doing a great job!

Please open a pull request for me @casperklein (don't worry about the contribution convention, and don't worry about the warning by CI, I will fix them).

PS: If your GITHUB_TOKEN exceeds the rate limit, you can change to a personal access token instead temporarily. See here

casperklein commented 3 years ago

This was just a quick hack/POC.

I took the the title and used it for the body text. I can provide a PR for that. I probably can hack something together, to make the Body text configurable like the title.

Even better would be, if the body contains the contributors that the PR is going to add:

This pull-request adds the following new contributors:

- new-contributor1
- new-contributor2

This however exceed my copy/paste/hack abilities 😄

Just let me know, if I should supply a PR with static/configurable body text, or if you think it's worth to list the new contributors like I wrote above and look yourself into it.

BobAnkh commented 3 years ago

Really appreciate this interesting feature!

Just open a PR with the static body text(which servers as a temporary solution).

I will take into consider the new FR and implement it when I am free(maybe it is not so easy to implement, for I have to know which contributors is new). Please open a new issue to talk about it. This will server as the content of the body text in the next version if I can implement it.

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue.