Fix automated Contributors.md update

[RNKuhns]

@achieveordie has been doing some really helpful work on automating the contributors.md update.

Looks like he ran into a problem. Just opening an issue for us to track the solution.

His original comment is below:

Whoops, it currently fails with the following:

[main 46fc2ca] [AUTOMATED] update CONTRIBUTORS.md
 Author: achieveordie <achieveordie@users.noreply.github.com>
 1 file changed, 53 insertions(+), 33 deletions(-)
INPUT_TAGGING_MESSAGE: 
No tagging message supplied. No tag will be added.
INPUT_PUSH_OPTIONS: 
remote: error: GH006: Protected branch update failed for refs/heads/main.        
remote: error: At least 1 approving review is required by reviewers with write access. 17 of 17 required status checks are expected.        
To https://github.com/sktime/sktime
 ! [remote rejected] main -> main (protected branch hook declined)
error: failed to push some refs to 'https://github.com/sktime/sktime'

Originally posted by @achieveordie in https://github.com/sktime/sktime/pull/3807#issuecomment-1382086216

[RNKuhns]

Am I understanding it right, that this fails b/c main is protected branch?

Could we just have it open a new PR?

These should be pretty easy approvals, so time cost of someone have to look at it and approve shouldn't be too burdensome.

[fkiraly]

should we consider this a duplicate of https://github.com/sktime/sktime/issues/2905 and just reopen #2905 on the basis that it is not fixed but ran into ramifications?

[fkiraly]

Am I understanding it right, that this fails b/c main is protected branch?

I don't "know" know, but given the error message it feels like a pretty safe bet.

[fkiraly]

Could we just have it open a new PR?

Could it instead just push to the branch that triggers it, pre-merge?

I think there was a discussion about complexities related to forks in the PR.

I'm fine if original PR or issue is re-opened in lieu of this. Only benefit of this is that it is easier to see. If we think we have a quick solution (and quick follow-on PR).

Otherwise copy over the comment from @achieveordie

Note that we might want to look at how pre-commit.ci is doing this (they automatically push black formatting changes as commit on PR. Even on PRs from fork branch).

[achieveordie]

We could follow the route recommended by the author of this GH Action.

More specifically (emphasis mine):

You need a Personal Access Token and you either have to allow force pushes or the Personal Access Token needs to belong to an Administrator. ... First, you have to create a new Personal Access Token (PAT), store the token as a secret in your repository and pass the new token to the actions/checkout Action step. ... Note: If you're working in an organisation, and you don't want to create the PAT from your personal account, we recommend using a bot-account for such tokens.

[achieveordie]

Could it instead just push to the branch that triggers it, pre-merge?

This is what I originally intended. This issue might be helpful in that regard. WDYT? @RNKuhns @fkiraly

[fkiraly]

I don't think it's a good idea to use a PAT with admin credentials, IT security-wise!

Because that exposes the PAT as a secret to alternative use by users with write access, and by using the admin PAT maliciously, users with lower security clearance could find a way gain admin credentials - PAT are not "purpose-bound".

I would prefer the solution where we push to the triggering branch, or at third branch that does not have branch protection.

[achieveordie]

I agree, especially since there's a safer alternative. I'll modify it to add pre-merge, though my changes would have to be merged for us to test it (see the link in my comment above to know why).

[fkiraly]

well, we'll have some weeks time to unmerge if it breaks anything, until 0.16.0

[achieveordie]

Turns out that things are (somewhat) working. See #4122 for reference.

In the current workflow, the on-push trigger is enabling update_contributors.yml workflow to run on the fork (one can see it is running only on the fork by seeing the repo the workflow checks out, not to mention the job is visible only in the fork)

I'm saying "somewhat" working since it doesn't include the latest changes but the ones made before. I could not find the three contributors in the built CONTRIBUTORS.md even though they have added themselves in .all-contributorsrc but I can see that my contributions have been reduced from changes to .all-contributorsrc in #3807.

[achieveordie]

@fkiraly @RNKuhns Apologies for letting this issue become stale, I've been incredibly busy with work lately. Do you think we can finish this up? As per my above comment, the current workflow is working, we just need to discuss if it working well.

If you need a refresher about the current status please let me know.

[fkiraly]

that would be appreciated - what would the required actions be? Is there something that needs approval or merge? The issue #4123 is still a draft (and says "do not merge"...)

[achieveordie]

Here's the current status:

1. The original PR #3807 was intended to work during the merge commit. The error shown in the top comment shows a problem with permissions.
2. To counter this, I proposed an alternative in #4123 where we check out the PR repo and run the workflow pre-merge. The author of this action recommended this method but it introduces a security vulnerability (hence a 'DO NOT MERGE' warning).
3. While I was trying to find a third viable alternative, I found that the current workflow is actually working, just not in the way we were expecting. a. If you open the linked PR (#4122), you'll see commits made by the bot. The workflow is running on the fork (and hence we can't see any workflow status here, but it is visible on their fork). b. There's a catch though, it doesn't include the latest addition, but the last-made changes (Example: Let's say that I change my contributions in .all-contributorsrc. In the following week, you add yourself in .all-constributorsrc in a different PR. It will be in this PR that the workflow will be triggered (on your fork) to update MY contributions in CONTRIBUTORS.md; changes corresponding to you will be reflected in the subsequent PR that makes changes to .all-contributorsrc).

What we need to discuss is if we want to rectify this catch. I don't know if there's a direct solution to this but we can consider this issue solved if we don't mind this small catch. We've already spent a lot of time on this addition, the ROI seems pretty low.

If you need further clarification, let me know.

[fkiraly]

I don't mind the catch, this is basically just a delay in the workflow run, right?

Does it mean the workflow that is currently on main is working, and we can close this issue as done?

[achieveordie]

I'd like to say that this issue can be closed but let me make a PR to test it and we can close it from there.