[DISCUSS] Pull Request (PR) check list

[Anilkumar Gingade <ag...@vmware.com>]

It is been some time we have been using a standard check-list for PRs. It may be time to look back and see if any of them were obsolete; and add new items based on the PR review experience.

Current PR check list items:

  1.  Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
  2.  Has your PR been rebased against the latest commit within the target branch (typically develop)?
  3.  Is your initial contribution a single, squashed commit?
  4.  Does gradlew build run cleanly?
  5.  Have you written or updated unit tests to verify your changes?
  6.  If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Based on how the PRs are created and with review requirement/process in place, the check-list from #1 - #5 seems to be obsolete.

  *   Ticket numbers are used/referred in PR
  *   The merging option shows if there is any conflict with base repo
  *   Unit tests are run on the PR and reviewers are expected to look for new/existing tests to be added/modified.

Adding some of the criteria we often miss out during the code changes could be more valuable to add into the checklist, e.g.:

  *   Any serialization changes done; requiring backward-compatibility/rolling-upgrade testing
  *   Is there any performance implication with these changes
  *   Was there any RFC for this PR and needs to be updated (link to the RFC)


Please share your thoughts/comments.

Thanks,
-Anil

Re: [DISCUSS] Pull Request (PR) check list

[Jacob Barrett <ja...@vmware.com>]

I honestly ignore the checklist.

> On Apr 8, 2021, at 2:24 PM, Anilkumar Gingade <ag...@vmware.com> wrote:
> 
> It is been some time we have been using a standard check-list for PRs. It may be time to look back and see if any of them were obsolete; and add new items based on the PR review experience.
> 
> Current PR check list items:
> 
>  1.  Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
>  2.  Has your PR been rebased against the latest commit within the target branch (typically develop)?
>  3.  Is your initial contribution a single, squashed commit?
>  4.  Does gradlew build run cleanly?
>  5.  Have you written or updated unit tests to verify your changes?
>  6.  If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
> Based on how the PRs are created and with review requirement/process in place, the check-list from #1 - #5 seems to be obsolete.
> 
>  *   Ticket numbers are used/referred in PR
>  *   The merging option shows if there is any conflict with base repo
>  *   Unit tests are run on the PR and reviewers are expected to look for new/existing tests to be added/modified.
> 
> Adding some of the criteria we often miss out during the code changes could be more valuable to add into the checklist, e.g.:
> 
>  *   Any serialization changes done; requiring backward-compatibility/rolling-upgrade testing
>  *   Is there any performance implication with these changes
>  *   Was there any RFC for this PR and needs to be updated (link to the RFC)
> 
> 
> Please share your thoughts/comments.
> 
> Thanks,
> -Anil
>