🌐 Github Issue | PR Review Policy #1296

New Issue

2019-09-11T04:40:02Z

ghbjklhv1 commented

2019-09-11 04:40:02 +00:00

(Migrated from github.com)

Basic Information

Short Description: Issue/PR Review and Inclusion Policy
Category: Contribution Guidelines

Description

I wanted to start a discussion on the basis of starting a review policy for PTIO.

What would this entail?
Generally speaking, review/auditing policies are used for security purposes.
In this case it would verify that any PR made to PTIO would be given effective time to be reviewed by not only the team but also the community.

# Basic Information **Short Description**: Issue/PR Review and Inclusion Policy **Category**: Contribution Guidelines # Description I wanted to start a discussion on the basis of starting a review policy for PTIO. ___________________ **What would this entail**? Generally speaking, review/auditing policies are used for security purposes. In this case it would verify that any PR made to PTIO would be given effective time to be reviewed by not only the team but also the community.

Mikaela commented

2019-09-11 08:05:59 +00:00

(Migrated from github.com)

I am not sure if this is a good idea and it's not enforceable.

There are at times hotfixes that are very small changes and just need to get through, and our GitHub configuration currently requires two approvals from a team member (it doesn't care about people outside of the team).

However I guess CONTRIBUTING.md could tell people to feel free to review PRs in the Files tabs, I recognise being unsure on whether I am "allowed" to do that in multiple projects.

@privacytoolsIO/editorial thoughts?

I am not sure if this is a good idea and it's not enforceable. There are at times hotfixes that are very small changes and just need to get through, and our GitHub configuration currently requires two approvals from a team member (it doesn't care about people outside of the team). However I guess CONTRIBUTING.md could tell people to feel free to review PRs in the Files tabs, I recognise being unsure on whether I am "allowed" to do that in multiple projects. @privacytoolsIO/editorial thoughts?

blacklight447 commented

2019-09-11 18:17:35 +00:00

(Migrated from github.com)

Well we could make a mandatory wait period of say 48 hours before a PR gets merged, so the community has some time to raise questions and review it themselves, with us of course still having the option to merge immediately to apply the above mentioned hot fixes in emergency situations

Mikaela commented

2019-09-11 21:51:18 +00:00

(Migrated from github.com)

Assigning @dawidpotocki also as they said to want to change possibly some things in contributing.md and maybe take this later.

Is there any method to integrate a wait period to GitHub? I don't like the idea of artificial limits though and I think most of PRs are slow to get merged regardless so the community would have time regardless. There is also the thing with @nitrohorse being from a significantly different timezone and one of the most active reviewers and is involved with most of merged pull requests.

Assigning @dawidpotocki also as they said to want to change possibly some things in contributing.md and maybe take this later. Is there any method to integrate a wait period to GitHub? I don't like the idea of artificial limits though and I think most of PRs are slow to get merged regardless so the community would have time regardless. There is also the thing with @nitrohorse being from a significantly different timezone and one of the most active reviewers and is involved with most of merged pull requests.

dawidpotocki commented

2019-09-11 23:21:25 +00:00

(Migrated from github.com)

I don't like idea of any waiting periods, it will change nothing.

Generally speaking, review/auditing policies are used for security purposes.

Our site is static, most of the time we are only editing HTML and CSS.
Really, what security issues could there be with them on most pull requests?
Sure, there is JavaScript, but we are not using it a lot and most of it
is actually 3rd party, very popular libraries like jQuery. In our case,
reviews are pretty much just for checking if it works, because malicious
stuff would be easily noticed. Also notice that most patches are done by
us and not community.

In this case it would verify that any PR made to PTIO would be given effective time to be reviewed by not only the team but also the community.

2 team members should be good enough for getting code merged into
master. Of course, I would love community reviewing our patches and
you can do it without being a member now, but making it mandatory would
mean that our changes would be stuck for some time and still very likely
we would get zero reviews from other people.

I don't like idea of any waiting periods, it will change nothing. > Generally speaking, review/auditing policies are used for security purposes. Our site is static, most of the time we are only editing HTML and CSS. Really, what security issues could there be with them on most pull requests? Sure, there is JavaScript, but we are not using it a lot and most of it is actually 3rd party, very popular libraries like jQuery. In our case, reviews are pretty much just for checking if it works, because malicious stuff would be easily noticed. Also notice that most patches are done by us and not community. > In this case it would verify that any PR made to PTIO would be given effective time to be reviewed by not only the team but also the community. 2 team members should be good enough for getting code merged into master. Of course, I would love community reviewing our patches and you can do it without being a member now, but making it mandatory would mean that our changes would be stuck for some time and still very likely we would get zero reviews from other people.

ghbjklhv1 commented

2019-09-12 01:31:35 +00:00

(Migrated from github.com)

2 team members should be good enough for getting code merged into master

Yes, this is good however PRs can get merged in just a few hours.
The current system would require a separate removal issue to be created.

There are at times hotfixes that are very small changes and just need to get through

In this case scenario you could make an exemption for spelling errors or under critical circumstances. These are very rare though and should be marked as minor edits.
Plus, generally spelling errors and whatnot don't cause much debate.

@DawidPotocki @blacklight447-ptio @mikaela

> 2 team members should be good enough for getting code merged into master Yes, this is good however PRs can get merged in just a few hours. The current system would require a separate removal issue to be created. ___________________ > There are at times hotfixes that are very small changes and just need to get through In this case scenario you could make an exemption for spelling errors or under critical circumstances. These are _very rare_ though and should be marked as minor edits. _Plus_, generally spelling errors and whatnot don't cause much debate. @DawidPotocki @blacklight447-ptio @mikaela

Mikaela commented

2019-09-12 21:12:21 +00:00

(Migrated from github.com)

Another GitHub issue: it's not possible to rerequest reviews from not-collaborators in the UI. The button just does nothing.

blacklight447 commented

2019-09-13 14:45:21 +00:00

(Migrated from github.com)

I think we can close this issue, as the way things get reviewed fine right now, never heard anyone complaining at least. we can always re open this issue if we deem it necessary.

This repo is archived. You cannot comment on issues.