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

Definition of done for Pull Requests #17486

Closed
benoitf opened this issue Jul 27, 2020 · 5 comments · Fixed by #17627
Closed

Definition of done for Pull Requests #17486

benoitf opened this issue Jul 27, 2020 · 5 comments · Fixed by #17627
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes

Comments

@benoitf
Copy link
Contributor

benoitf commented Jul 27, 2020

Is your task related to a problem? Please describe.

Here is an updated PRs Approval Checklist:

The things a reviewer needs to verify before approving a PR (in italic those checks that can/should be automated):

  1. Code produced (No TODO items on the code, tickets are created for them)
  2. Code documented
  3. Builds without errors
  4. Doesn’t break self-hosting
  5. Static code analysis done and issues fixed
  6. Unit tests written and passing
  7. How the PR has been tested (which env) and how a reviewer can test it too (if possible, a button to check online)?
  8. Deployed to test/staging environment and passed e2e happy path test
  9. Code review done
  10. Any build/deployment/configuration changes are implemented, tested, documented and communicated (Operational documentation)
  11. Relevant documentation produced and/or updated (User documentation)
  12. Video Recording of the fix or feature (optional)
  13. Link to corresponding issues (if any)
    Has a milestone and a type (bug/feature/task) or is linked to issue with milestone and type

This definition of done applies to all the work that is merged into the Che code repositories. Therefore it should be applied to all Pull Requests and is part of the code review. It is both reviewers’ and the pull request creators’ responsibility to ensure that it has been followed..

@benoitf benoitf added the kind/task Internal things, technical debt, and to-do tasks to be performed. label Jul 27, 2020
@benoitf
Copy link
Contributor Author

benoitf commented Jul 27, 2020

Here is an example of what could be done for PR (updating template)

benoitf/che-template-issues#1

@che-bot che-bot added the status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. label Jul 27, 2020
@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Jul 27, 2020

@benoitf, @l0rd: WDYT about dividing the list by mandatory and optional sections to simplify PR approval decision?

Mandatory:

(1). Code produced (No TODO items on the code, tickets are created for them)
(2). Code documented
(3). Builds without errors
(4). Doesn’t break self-hosting
(5). Static code analysis done and issues fixed
(6). Unit tests written and passing
(7). How the PR has been tested (which env) and how a reviewer can test it too (if possible, a button to check online)?
(8). Deployed to test/staging environment and passed e2e happy path test
(9). Code review done
(14). Has a milestone and a type (bug/feature/task) or is linked to issue with milestone and type

Optional:

(10). Any build/deployment/configuration changes are implemented, tested, documented and communicated (Operational documentation)
(11). Relevant documentation produced and/or updated (User documentation)
(12). Video Recording of the fix or feature (optional)
(13). Link to corresponding issues (if any)

@ericwill ericwill added area/dev-experience and removed status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. labels Jul 27, 2020
@l0rd
Copy link
Contributor

l0rd commented Jul 27, 2020

@dmytro-ndp I think that we should distinguish between points that can be mandatory for the the particular PR (docs updates for example) and points that are always optional (video recording).

I would remove optionals (video recording) from the the checklist since it's mentioned in the description (screen capture or screencast).

@l0rd
Copy link
Contributor

l0rd commented Jul 27, 2020

@benoitf thinking about this afternoon discussion I am wondering if we should not provide a more compact checklist and then provide detailed explanation in the wiki.

For example:

## What does this PR do?
...(with sceenshots or screencast)...

## The PR Checklist

As the author of this PR I made sure that:

 - [ ] The Eclipse Contributor Agreement has been signed
 - [ ] Code produced is complete
 - [ ] Code builds without errors
 - [ ] A corresponding automated test is added for every bug fixed
 - [ ] The devfile is up to date and works
 - [ ] The PR has been tested and "How to test" instructions are provided below
 - [ ] Link to any corresponding issue and PR is provided below
 - [ ] Relevant user documentation updated
 - [ ] Relevant contributing documentation is updated
 - [ ] CI/CD changes implemented, documented and communicated

A detailed explanation of the PR checklist is available in the Development Process section of the Eclipse Che wiki. 

## How to test this PR
(...)

## Corresponding issues and PRs
(...)

@l0rd
Copy link
Contributor

l0rd commented Jul 27, 2020

Other enhancements:

  • The list doesn't need to be numbered
  • Checkboxes should only be for the PR checklist, not for runtime/installation method/environment otherwise:

image

@l0rd l0rd added the new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes label Jul 28, 2020
benoitf added a commit to benoitf/che that referenced this issue Aug 13, 2020

Verified

This commit was signed with the committer’s verified signature.
benoitf Florent BENOIT
…on of done on PR

eclipse-che#17486

Change-Id: I34dd7dead00e75dafd905b632bd24b81a9be24f1
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
benoitf added a commit to benoitf/che that referenced this issue Sep 7, 2020

Verified

This commit was signed with the committer’s verified signature.
benoitf Florent BENOIT
…on of done on PR

eclipse-che#17486

Change-Id: I34dd7dead00e75dafd905b632bd24b81a9be24f1
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
benoitf added a commit to benoitf/che that referenced this issue Sep 7, 2020

Verified

This commit was signed with the committer’s verified signature.
benoitf Florent BENOIT
…on of done on PR

eclipse-che#17486

Change-Id: I34dd7dead00e75dafd905b632bd24b81a9be24f1
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
benoitf added a commit to benoitf/che that referenced this issue Sep 11, 2020

Verified

This commit was signed with the committer’s verified signature.
benoitf Florent BENOIT
…on of done on PR

eclipse-che#17486

Change-Id: I34dd7dead00e75dafd905b632bd24b81a9be24f1
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
benoitf added a commit that referenced this issue Sep 11, 2020
…on of done on PR

#17486

Change-Id: I34dd7dead00e75dafd905b632bd24b81a9be24f1
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants