search

openssl / general-policies

Mirror of the repository for general policies, governed by the OMC (OpenSSL Management Committee)
13 stars 23 forks source link

Create a Feature Branch Approval Policy #62

Closed mattcaswell closed 4 months ago

mattcaswell commented 5 months ago

Defines a policy for how new feature branches should be requested and approved. We defer the management of feature branches to other future policies.

Fixes openssl/project#423

mattcaswell commented 5 months ago

See openssl/project#423

t8m commented 5 months ago

LGTM

hlandau commented 5 months ago

Seems reasonable to me. I think it needs to discuss rebasing a bit, I don't think that can be deferred.

The protocol I proposed when creating the QUIC server feature branch was that rebasing can be done immediately after getting an informal OK from another committer.

However, we need to determine how to review nontrivial rebases where changes had to be made during the rebase process.

As with the QUIC server feature branch feature branches should be named feature/foo.

Other than that, LGTM

hlandau commented 5 months ago

Hmm, though. I'm not entirely 100% about saying a feature branch can be merged without further review. I think it should be done as part of a PR, even if we agree that we don't actually need to go and review all the changes in that PR because the contents have already been reviewed individually. This will be a PR between two branches in the main repository, rather than the main repository and someone's fork. It can then be given a simple procedural approval in the usual manner, basically just glancing at it to make sure there's nothing off about the changes (e.g. if a PR to merge a QUIC feature branch had changes to crypto/aes, there's obviously something weird there). It would be similar to the current rather rudimentary review and approval of fuzz-corpora changes. Also ensures it is confirmed that it has been determined that it is OK to merge a feature branch at that particular time with commitment to a release. Don't think we need a 24 hour timer though, again like fuzz-corpora.

mattcaswell commented 5 months ago

Update pushed. Please take another look

mattcaswell commented 5 months ago

Updated again to resolve @t8m's nit.

ghost commented 5 months ago

We will have to discuss this next week - with Anton.

mattcaswell commented 5 months ago

Policy proposal has been announced on openssl-project

rsbeckerca commented 5 months ago

Does this policy permit community supported platforms? I would not want to see platform-specific features, but there are situations where code has to be added through a PR to support a platform's ongoing participation in OpenSSL.

mattcaswell commented 5 months ago

Does this policy permit community supported platforms? I would not want to see platform-specific features, but there are situations where code has to be added through a PR to support a platform's ongoing participation in OpenSSL.

The policy says nothing about what type of features can be added through feature branches, and whether or not they are platform-specific. In general most platform-specific changes tend to be relatively small tweaks here and there which can usually be contained within a single PR (in which case this policy is not relevant). If however a larger change was necessary for something platform specific, and that change was too large for a single PR, then this policy could be used for it.

ghost commented 5 months ago

@mattcaswell We have to wait another week to vote. Let´s update next week.

fwh-dc commented 4 months ago

What is the policy for reviews of pull request to a feature branch?

Would a pull request with failing tests be acceptable in some scenarios for example?

t8m commented 4 months ago

There is no policy yet. However when the feature branches were discussed the general consensus at that time was that for the PRs on feature branches the same criteria should apply as for the master branch.

mattcaswell commented 4 months ago 
Topic: Accept the feature branch approval policy as of d62052487a0656d80b4a5972625bedf06e20c94f
Proposed by: Matt Caswell
Issue link: https://github.com/openssl/general-policies/pull/62
Public: yes
Opened: 2024-02-20
Closed:
Accepted:  yes/no  (for: 0, against: 0, abstained: 0, not voted: 0)

  Anton      [  ]
  Kurt       [  ]
  Matt       [+1]
  Richard    [  ]
  Tim        [  ]
mattcaswell commented 4 months ago

I have called this to vote on openssl-project

levitte commented 4 months ago

Vote: +1

t-j-h commented 4 months ago

Vote: +1

arapov commented 4 months ago

Vote: +1

mattcaswell commented 4 months ago

@kroeckx - please could you vote?

mattcaswell commented 4 months ago

I have closed this vote and pushed this PR:

Topic: Accept the feature branch approval policy as of d62052487a0656d80b4a5972625bedf06e20c94f
Proposed by: Matt Caswell
Issue link: https://github.com/openssl/general-policies/pull/62
Public: yes
Opened: 2024-02-20
Closed: 2024-02-29
Accepted:  yes  (for: 4, against: 0, abstained: 0, not voted: 1)

  Anton      [+1]
  Kurt       [  ]
  Matt       [+1]
  Richard    [+1]
  Tim        [+1]