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

Add @allcontributors bot #1126

Closed
sreichel opened this issue Jul 26, 2020 · 25 comments · Fixed by #1262
Closed

Add @allcontributors bot #1126

sreichel opened this issue Jul 26, 2020 · 25 comments · Fixed by #1262

Comments

@sreichel
Copy link
Contributor

sreichel commented Jul 26, 2020

Days ago @kkrieger85 manually added a contributors list (#1069) ... to keep it uo2date someone should install the bot.

Blocks #1099

@kkrieger85
Copy link
Contributor

kkrieger85 commented Jul 27, 2020

@all-contributors please add kkrieger85 for docs

@kkrieger85
Copy link
Contributor

A request to install AllContributors has been submitted on the @OpenMage account.

@sreichel
Copy link
Contributor Author

@kkrieger85 i submitted same request weeks ago (with your PR) ...

Want to write a RFC? :P

@kkrieger85
Copy link
Contributor

AFAIK @Flyingmana already added this bot. But now, I'm not sure anymore 🙈

@Flyingmana
Copy link
Contributor

@kkrieger85 not me, I dont have the permission to add bots/integrations to the Organisation.
But if it said a request was submitted, everyone who has this permission should have gotten a notification mail.

@sreichel
Copy link
Contributor Author

I dont have the permission to add bots/integrations to the Organisation.

Thought you had full access. If only @LeeSaferite or @drobinson could do that, it would be bad.

@drobinson
Copy link
Collaborator

drobinson commented Jul 31, 2020 via email

@drobinson
Copy link
Collaborator

drobinson commented Jul 31, 2020

Also, can anyone speak to the security of this bot? Feels like it needs some validation before we install an app that gets full write access to the repository.

@Flyingmana
Copy link
Contributor

I would (because of the security topic) prefer something, which would work based on PRs (like the dependabot and greenkeeper and similar)

@sreichel
Copy link
Contributor Author

sreichel commented Aug 3, 2020

I would (because of the security topic) prefer something, which would work based on PRs (like the dependabot and greenkeeper and similar)

This is what this bot does.

Enter the @all-contributors bot! The bot will automatically pull a user's profile, grab the contribution type emoji, update the project README and then open a Pull Request against the project

(https://allcontributors.org/docs/en/bot/overview)

Dependabot need same (even more) rights ...

All-contributers:

  • Write access to code
  • Read access to metadata
  • Read and write access to issues and pull requests

Dependabot:

  • Write access to code
  • Read access to checks, commit statuses, and metadata
  • Read and write access to issues and pull requests

@sreichel
Copy link
Contributor Author

sreichel commented Aug 3, 2020

Which repositories need this bot? Is it just LTS?

I like the idea behind this bot ... I'd install it for all repos.

@kkrieger85
Copy link
Contributor

I gave up with all-contributors bot. Will add instructions on how to add self to contributors

@sreichel
Copy link
Contributor Author

sreichel commented Oct 18, 2020

@drobinson this bot does not harm. It creates PRs to update README.md.

@sreichel
Copy link
Contributor Author

sreichel commented Jan 13, 2021

@LeeSaferite @drobinson bump. Can you please grant rights for @Flyingmana to add such things?

@sreichel
Copy link
Contributor Author

Bump.

@Flyingmana do you still have any concerns?

Either we add this harmles bot or we should remove it from README,md completly ...

IMHO I'm happy about every new contributor, every reported issue or PR. This would just be a little change or chance to thank everybody helping here. Please (let) add this bot.

@Flyingmana
Copy link
Contributor

Flyingmana commented Feb 17, 2021

Iam still not the one who has to decide this.

but looking at
all-contributors/all-contributors#284
and
all-contributors/app#147
Iam confident to say, I dont trust them to have write access.

And even as it should just do something harmless, we would give it great power.
And we had just recently a case of "something harmless" turning into a "serious Threat" among chrome extensions.
https://www.xda-developers.com/google-chrome-the-great-suspender-malware/

but I would still like to keep the functionality, which as far as I see consists of filling this file: https://github.com/OpenMage/magento-lts/blob/1.9.4.x/.all-contributorsrc
and using this file to render the list in the Readme.md, right?
The rendering can be easy replaced with a simple php script.

the adding of users with the https://allcontributors.org/docs/en/bot/usage#all-contributors-add could be easy replicated with a a smaller tool using the github graphql api to look for such comments, similar to what I did recently for the changelog generator looking for PullRequests.

What other features would we need to keep in mind?

@sreichel
Copy link
Contributor Author

sreichel commented Feb 27, 2021

I gave up with all-contributors bot.

Me too :(

Iam still not the one who has to decide this.

Don't you want to decide that, or don't you have the permissions to install a bot? The latter would be bad.

Iam confident to say, I dont trust them to have write access.

Honestly, that sounds paranoid. What is the bot supposed to do? Inject malicious code?

There are things that are important, so I'm closing this again now.

@kkrieger85
Copy link
Contributor

IMHO this is a punch in every contributors face.

But I will accept the group (non) decision.

@sreichel
Copy link
Contributor Author

sreichel commented Mar 6, 2021

@kkrieger85 i wrote emails to @LeeSaferite and @drobinson, but got no response. If @Flyingmana cant/wont decide it, who is responsible for such things?

@kkrieger85
Copy link
Contributor

@sreichel I did not intend to blame you or someone else.

In this case: We should ask ourselfes: If people don't respond, how can this work if there is something important to decide.
Does it need more people with Admin-rights? Does it need a fallback for the case, that "important" people don't respond?

@sreichel
Copy link
Contributor Author

sreichel commented Mar 6, 2021

but I would still like to keep the functionality, which as far as I see consists of filling this file: https://github.com/OpenMage/magento-lts/blob/1.9.4.x/.all-contributorsrc
and using this file to render the list in the Readme.md, right?
The rendering can be easy replaced with a simple php script.

Right. But i still do not understand why you want to reinvent the wheel.

Also a custom script would requiere write access to create PRs. Can your "changelog-script" directly push to our "main branches"? Where is the difference to that bot? Our branches are protected, direct merges should not be possible and should require reviews.

You mentioned dependabot, that exactly works the same way and should also be considered as "harmfull", but this is suggested (and widly used).

I agree with @kkrieger85 ... a slap in the face to any contributor. I don't know who has what rights, but when we have to rely on @LeeSaferite or @drobinson who haven't been active for years, something is wrong.

From https://github.com/orgs/OpenMage/people it looks like only 3 people have superior permissions.

During last weeks we had some urgent PRs (rollbacks, broken packagist), but it took "ages" to get it merged.

Does it need more people with Admin-rights?

No sure. If we can react in time, i guess not. But if we cannot ...

@Flyingmana
Copy link
Contributor

To make it short, giving allcontributors bot write access, opens us up to one more supply chain attack.

Should we ever get to be reviewed in context of PCI, this kind of things will go into the risk analysis.
Its a necessary to look at this things from a paranoid point of view, and to reduce our attack surface proactively, and not reactively(only after something happened)

By "reinventing the wheel", we can either remove a supply chain for this completely, or can rely on a trusted (trusted in the sense of equal or higher security standards than ours) supply chain (like for example the symfony components)

@sreichel
Copy link
Contributor Author

sreichel commented Mar 7, 2021

i can understand your basic thought about security, but i can't follow your argumentation. where is the difference between the bot and your other solution? both need write permissions for the PRs. and neither of them should have write permissions directly.

in the end, you seem to have made the decision.

if you like to write such a script i would be happy, but i don't really see an advantage until the permission management in git is more granular.

@addison74
Copy link
Contributor

We should know the people we could count on to move forward with this project. If some of them are not responding in a reasonable time it is clear we cannot count on them. Personally I have a bad experience with Turpentine project where the owners abandoned the project even there were many interested to continue it. What a great extension to improve Magento performance.

@Flyingmana
Copy link
Contributor

Ok, as we drift away from the main topic, I will lock this conversation.
Please use the board/Discussions, or open another matching Issue.

If you want to decide something on a higher level, or you want to overrule my Veto as a maintainer for this, both can be achieved via an RFC.

@OpenMage OpenMage locked as off-topic and limited conversation to collaborators Mar 8, 2021
Flyingmana pushed a commit that referenced this issue Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.