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

doc: changes to merge criteria (4 eyes principle) #62122

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

nashif
Copy link
Member

@nashif nashif commented Aug 30, 2023

  • doc: roles: remove reference to slide
  • doc: release: update merge criteria

Fixes #61358

@nashif nashif added the TSC Topics that need TSC discussion label Aug 30, 2023
@nashif nashif requested a review from a team August 30, 2023 22:56
Comment on lines 330 to 333
* Release engineers shall not merge code changes originating from the same
organisation if all other reviewers are also from the same organisation.
To be able to merge such changes, at least one review shall be from a
different organisation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about ?
A change coming from one organisation needs at least an approval from another organisation to be merged

@nashif nashif requested a review from a team September 1, 2023 19:35
doc/project/project_roles.rst Outdated Show resolved Hide resolved
doc/project/project_roles.rst Show resolved Hide resolved
doc/project/project_roles.rst Outdated Show resolved Hide resolved
doc/project/project_roles.rst Outdated Show resolved Hide resolved
doc/project/project_roles.rst Show resolved Hide resolved
submitters' organisation).
* Changes or additions to hardware support (driver, SoC, boards) shall at
least have the merger be from a different organisation.
* Release engineers shall not merge code changes originating from the same
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not apply for certain types of changes in my opinion. There are just too many changes that are very difficult to review for someone who is not familiar with the hardware or area. I would again differentiate between introducing new things and maintaining them.

doc/project/project_roles.rst Outdated Show resolved Hide resolved
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, it is looking much better with this phrasing.
A few more minor things.

doc/project/project_roles.rst Outdated Show resolved Hide resolved
doc/project/project_roles.rst Show resolved Hide resolved
doc/project/project_roles.rst Outdated Show resolved Hide resolved
doc/project/project_roles.rst Outdated Show resolved Hide resolved
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just very minors at this point from me.

doc/project/project_roles.rst Outdated Show resolved Hide resolved
doc/project/project_roles.rst Outdated Show resolved Hide resolved
doc/project/project_roles.rst Outdated Show resolved Hide resolved
doc/project/project_roles.rst Outdated Show resolved Hide resolved
Reference to slide that does not exist.

Signed-off-by: Anas Nashif <[email protected]>
Document 4 eye principal for reviews and merges.

Signed-off-by: Anas Nashif <[email protected]>
each affected area; Unless the changes to a given area are considered trivial
enough, in which case approvals by other affected subsystems
maintainers/collaborators would suffice.
* Four eye principle on the organisation level. We already require at least 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Four eye principle on the organisation level. We already require at least 2
* Four eyes principle on the organisation level. We already require at least 2
approvals (basic four eyeS principle), however, such reviews and approvals

one review shall be from a different organisation.

* A minimum review period of 2 business days, 4 hours for trivial changes (see
:ref:`review_time`).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find these very short but off-topic...

@nashif nashif removed the TSC Topics that need TSC discussion label Sep 27, 2023
@nashif nashif merged commit 79aaa90 into zephyrproject-rtos:main Sep 28, 2023
14 of 15 checks passed
@nashif nashif deleted the 4eye branch September 28, 2023 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Define guidelines for improved four eyes principle