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

RFC-0053: defining pull-request workflow #53

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions rfcs/0053-pull-request-workflow.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
---
feature: pull-request workflow
start-date: 2019-09-28
author: Ingolf Wagner (@mrVanDalo)
co-authors: lassulus (@lassulus)
shepherd-team: (names, to be nominated and accepted by RFC steering committee)
shepherd-leader: (name to be appointed by RFC steering committee)
related-issues:
---

# Summary
[summary]: #summary

Pull-Request on GitHub are the main way we improve our code base.
This Document should settle everything that needs to be done
to bring code upstream in the
[Nixpkgs Repository](https://github.com/nixos/nixpkgs/).

# Motivation
[motivation]: #motivation

Eliminate questions from contributors and maintainers
about what should be done next and who should do it.
This is not a new approach, it is more a settlement
on how we do it now.

# Detailed design
[design]: #detailed-design

Define all steps of a Pull-Request.
Use Roles to define responsibilities in every step.

## Roles
[roles]: #roles

Everybody involved in the process of contributing has one or multiple
of the following roles

* Contributor
* Bot
* Reviewer
* Maintainer (has merge privileges)
mrVanDalo marked this conversation as resolved.
Show resolved Hide resolved

The responsibilities of theses roles are defined in the rest of this RFC.
mrVanDalo marked this conversation as resolved.
Show resolved Hide resolved

## States of a Pull-Request
[state]:#states

This diagram defines all states of a Pull-Request,
and their transitions to other states.

![pull-request state](0053-pull-request-workflow/pull-request-states.svg)

## Responsibilities and Actions
[responsibilities]:#responsibilities

The responsibilities of every role is defined by the following diagram:

![pull-request activity](0053-pull-request-workflow/pull-request-roles.svg)
Copy link
Member

Choose a reason for hiding this comment

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

It is a common practice that the merger, will backport changes without having a backport pull request.

Copy link
Author

Choose a reason for hiding this comment

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

The pull-request is created in by the contributor in the 5th line, right below the bar.


## About Pull-Request

### Packages

* contributor must decide if a Backport is necessary
mrVanDalo marked this conversation as resolved.
Show resolved Hide resolved
* after the Pull-Request to `master`, `staging` or `staging-next` is merged,
the Backport Pull-Request is created
* Backport Pull-Requests must be linked to the original Pull-Requests (using `git cherry-pick -x`).
* reviewer and maintainer can deny the Backport
mrVanDalo marked this conversation as resolved.
Show resolved Hide resolved

### Modules

* modules should have tests
* new modules must have tests
Copy link
Member

Choose a reason for hiding this comment

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

Tbh I'm not sure if that rule is too strict: I remember several cases where I actually wanted to write a test, but failed to find a suitable approach. One example that comes to my mind right now would be the autorandr module where you'd have to simulate several monitors in the test VM.

I'd rather go with "reviewers should encourage contributors to write tests for new modules" to make it clear that we want to have tests but if there's no way around this, it's still fine to skip that task.

Copy link
Author

Choose a reason for hiding this comment

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

True, it is not possible to create module tests for everything.

reviewers should encourage contributors to write tests for new modules

Implies it is the responsibility of the reviewer to make sure tests are written (if possible). I can live with that.

Copy link
Member

Choose a reason for hiding this comment

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

I guess another approach is to say that «new modules should have tests or an explicit description of problems with testing (e.g. hardware interaction)»

* modules should not be Backported
mrVanDalo marked this conversation as resolved.
Show resolved Hide resolved
* Backports of modules must be pretty good argued.
mrVanDalo marked this conversation as resolved.
Show resolved Hide resolved

## Links

* [How to write Module Tests](https://nixos.org/nixos/manual/index.html#sec-nixos-tests)
* [Contribution Guidelines](https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md)

# Unresolved questions
[unresolved]: #unresolved-questions

* The Pull-Request of a Backport should be created by the bot.
But if that is the case, the original contributor might not be able
to make changes on the branch behind the Pull-Request.
Copy link
Member

Choose a reason for hiding this comment

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

Random idea: how about something like @ofborg backport 19.09?

Copy link
Author

Choose a reason for hiding this comment

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

The problem is the bot would create the pull-request and the contributor must be able to edit it.
There are two backport possibilities

  • cherry pick
  • new pull-request, because the software differs in the stable branches.

Copy link
Member

Choose a reason for hiding this comment

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

And the main catch is what happens if person A creates a PR and person B requests a backport…

* Backports without changes in master are not discussed.
Copy link
Member

Choose a reason for hiding this comment

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

I think the PR should include this. In case the update cannot go through master/staging/staging-next it should be made directly against release-YY.MM or staging-YY.MM.

Copy link
Author

Choose a reason for hiding this comment

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

So this would follow the rules of a normal pull-request, just to a different branch?

Copy link
Member

Choose a reason for hiding this comment

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

And with an extra sentence about the reasons it's irrelevant for master

Copy link
Author

Choose a reason for hiding this comment

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

@FRidh I think I should also put some more information about the branch structure we have. Bad thing is that I don't know the way we use branches. I know the master and release branches. But I don't know what the other branches in https://github.com/nixos/nixpkgs/ are for and how they interact with each other.

These branches look like I should write about them:

  • security-updates : I have no clue how this branch work
  • staging-next : I guess this is the branch that will be the next release. But what is staging than for? I guess your pull-request goes to master, and than you backport to staging-next and staging.
  • staging : I guess this is the branch that will be the next release. But what is staging-next than for? I guess your pull-request goes to master, and than you backport to staging-next and staging.
  • master : I put my pull-request always against master.
  • staging-yy.mm : Hmm we have staging and staging-next why do we need a staging-19.09 ?
  • release-yy.mm : I guess this is the branch which tracks the so called stable channels (nixpkgs-19.03, nixpkgs-19.09, ... ).

Did I miss one?

Copy link
Author

Choose a reason for hiding this comment

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

Ah I found #26 I will read it and come back to this comment.

for example security patches that only affect older versions in stable.

# Future work
[future]: #future-work

* The Pull-Request template needs an option "Backport needed?"
mrVanDalo marked this conversation as resolved.
Show resolved Hide resolved
* Add a link to this document in the
[Contribution Guidelines](https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md)
1 change: 1 addition & 0 deletions rfcs/0053-pull-request-workflow/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.history
672 changes: 672 additions & 0 deletions rfcs/0053-pull-request-workflow/pull-request-roles.graphml

Large diffs are not rendered by default.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
238 changes: 238 additions & 0 deletions rfcs/0053-pull-request-workflow/pull-request-roles.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
311 changes: 311 additions & 0 deletions rfcs/0053-pull-request-workflow/pull-request-states.graphml

Large diffs are not rendered by default.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
211 changes: 211 additions & 0 deletions rfcs/0053-pull-request-workflow/pull-request-states.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
16 changes: 16 additions & 0 deletions rfcs/0053-pull-request-workflow/shell.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
with import <nixpkgs> {};

stdenv.mkDerivation rec {

name = "rfc-53";

env = buildEnv {
name = name;
paths = buildInputs;
};

buildInputs = [
yed
];

}