[DISCUSS] Template to be used for Pull Requests

[Wilder Rodrigues <WR...@schubergphilis.com>]

Hi all,

Since I became committer I have been doing a bit more of reviews/tests on PRs with the goal to increase CloudStack quality. Sometimes it’s a bit difficult to review/test a PR due to lack of informations concerning the author's environment, tests that were executes, steps to tests the PR, etc. So, here I come, with a proposal to have a template for PRs.

A PR should contain:

* A title including the ACS issue ID - if any
* A clear description of what has been fixed and how
* A description of the environment where the PR has been tested by the author of the PR
  - KVM/XenServer/HyperV/etc
  - Operating System
  - Storage type
  - Is hypervisor + Manegement Server + Database in the same machine?

* The tests that have been executed on that same environment by the author of the PR
   - Full Maven build including tests execution?
   - New unit tests added? (only if Java code was changed)
   - New integration tests added/needed?
   - Any integration test executed? If so, which ones?
   - Any manual tests?

* Proposal of the steps to be taken by the reviewers of the PR in order to test it
  - How to reproduce the behvaiour before the the PR
  - How to test the changes after applyting the PR

The list above might look a bit too much. But this will help the reviewers to get confidence on the changes being proposed.

In addition, I would like to suggest that if a PR does not contain at least: 
  - test environment details
  - full Maven build + unit tests
  - manual tests to prove that the fix works
  - it should not be merged - unless the change is about a typo or a shell script line that can be tested on a terminal console.

What are your thoughts on that?

Cheers,
Wilder

Re: [DISCUSS] Template to be used for Pull Requests

[Daan Hoogland <da...@gmail.com>]

Wilder, Your report have been very extensive. they are useful for the
impressive works you contribute. maybe they are a bit overwhelming for the
beginner that contributes little improvements. I would like to change the *
and - list item denoters into + or - to signify optionality. I think for a
simple fix (typo's and such) at the most 3 +s should remain.

You overall idea is good and the frustration from which it arose very valid.
€0,02

Op wo 3 jun. 2015 om 10:07 schreef Wilder Rodrigues <
WRodrigues@schubergphilis.com>:

> Hi all,
>
> Since I became committer I have been doing a bit more of reviews/tests on
> PRs with the goal to increase CloudStack quality. Sometimes it’s a bit
> difficult to review/test a PR due to lack of informations concerning the
> author's environment, tests that were executes, steps to tests the PR, etc.
> So, here I come, with a proposal to have a template for PRs.
>
> A PR should contain:
>
> * A title including the ACS issue ID - if any
> * A clear description of what has been fixed and how
> * A description of the environment where the PR has been tested by the
> author of the PR
>   - KVM/XenServer/HyperV/etc
>   - Operating System
>   - Storage type
>   - Is hypervisor + Manegement Server + Database in the same machine?
>
> * The tests that have been executed on that same environment by the author
> of the PR
>    - Full Maven build including tests execution?
>    - New unit tests added? (only if Java code was changed)
>    - New integration tests added/needed?
>    - Any integration test executed? If so, which ones?
>    - Any manual tests?
>
> * Proposal of the steps to be taken by the reviewers of the PR in order to
> test it
>   - How to reproduce the behvaiour before the the PR
>   - How to test the changes after applyting the PR
>
> The list above might look a bit too much. But this will help the reviewers
> to get confidence on the changes being proposed.
>
> In addition, I would like to suggest that if a PR does not contain at
> least:
>   - test environment details
>   - full Maven build + unit tests
>   - manual tests to prove that the fix works
>   - it should not be merged - unless the change is about a typo or a shell
> script line that can be tested on a terminal console.
>
> What are your thoughts on that?
>
> Cheers,
> Wilder