sigs.k8s.io/cluster-api@v1.6.3/REVIEWING.md (about)

     1  # Code Review in Cluster API
     2  
     3  ## Goal of this document
     4  
     5  - To help newcomers to the project in implementing better PRs given the knowledge of what will be evaluated 
     6    during the review.
     7  - To help contributors in stepping up as a reviewer given a common understanding of what are the most relevant
     8    things to be evaluated during the review.
     9  
    10  IMPORTANT: improving and maintaining this document is a collaborative effort, so we are encouraging constructive
    11  feedback and suggestions.
    12  
    13     * [Code Review in Cluster API](#code-review-in-cluster-api)
    14        * [Goal of this document](#goal-of-this-document)
    15        * [Resources](#resources)
    16        * [Definition](#definition)
    17           * [Controller reentrancy](#controller-reentrancy)
    18           * [API design](#api-design)
    19              * [Serialization](#serialization)
    20              * [Owner References](#owner-references)
    21           * [The Cluster API contract](#the-cluster-api-contract)
    22           * [Logging](#logging)
    23           * [Testing](#testing)
    24  
    25  ## Resources
    26  
    27  - [Writing inclusive documentation](https://developers.google.com/style/inclusive-documentation)
    28  - [Contributor Summit NA 2019: Keeping the Bar High - How to be a bad-ass Code Reviewer](https://www.youtube.com/watch?v=OZVv7-o8i40)
    29  - [Code Review Developer Guide - Google](https://google.github.io/eng-practices/review/)
    30  - [The Gentle Art Of Patch Review](https://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/)
    31  
    32  ## Definition
    33  
    34  (from [Code Review Developer Guide - Google](https://google.github.io/eng-practices/review/)) 
    35  
    36  _"A code review is a process where someone other than the author(s) of a piece of code examines that code"_
    37  
    38  Within the context of cluster API the following design items should be carefully evaluated when reviewing a PR:
    39  
    40  ### Controller reentrancy
    41  
    42  In CAPI most of the coding activities happen in controllers, and in order to make robust controllers,
    43  we should strive for implementing reentrant code.
    44  
    45  A reentrant code can be interrupted in the middle of its execution and then safely be called again
    46  ("re-entered"); this concept, applied to Kubernetes controllers, means that a controller should be capable
    47  of recovering from interruptions, observe the current state of things, and act accordingly. e.g.
    48   
    49  - We should not rely on flags/conditions from previous reconciliations since we are the controller
    50    setting the conditions. Instead, we should detect the status of things through introspection at
    51    every reconciliation and act accordingly.
    52  - It is acceptable to rely on status flags/conditions that we've previously set as part
    53    of the current reconciliation.
    54  - It is acceptable to rely on status flags/conditions set by other controllers.
    55  
    56  NOTE: An important use case for reentrancy is the move operation, where Cluster API objects gets moved
    57  to a different management cluster and the controller running on the target cluster has to
    58  rebuild the object status from scratch by observing the current state of the underlying infrastructure.
    59  
    60  ### API design
    61  
    62  The API defines the main contract with the Cluster API users. As most of the APIs in Kubernetes,
    63  each API version encompasses a set of guarantees to the user in terms of support window, stability,
    64  and upgradability. 
    65  
    66  This makes API design a critical part of Cluster API development and usually:
    67  
    68  - Breaking/major API changes should go through the CAEP process and be strictly synchronized with the major
    69    release cadence.
    70  - Non-breaking/minor API changes can go in minor releases; non-breaking changes are generally:
    71    - additive in nature
    72    - default to pre-existing behavior
    73    - optional as part of the API contract
    74  
    75  On top of that, following API design considerations apply.
    76  
    77  #### Serialization
    78  
    79  The Kubernetes API-machinery that is used for API serialization is build on top of three
    80  technologies, most specifically:
    81  
    82  - JSON serialization
    83  - Open-API (for CRDs)
    84  - the go type system
    85  
    86  One of the areas where the interaction between those technologies is critical in the handling of optional
    87  values in the API; also the usage of nested slices might lead to problems in case of concurrent
    88  edits of the object.
    89  
    90  #### Owner References
    91  
    92  Cluster API leverages the owner ref chain of objects for several tasks, so it is crucial to evaluate the
    93  impacts of any change that can impact this area. Above all:
    94  
    95  - The delete operation leverages on the owner ref chain for ensuring the cleanup of all the resources when
    96    a cluster is deleted; 
    97  - clusterctl move uses the owner ref chain for determining which object to move and the create/delete order.
    98  
    99  ### The Cluster API contract
   100  
   101  The Cluster API rules define a set of rules/conventions the different provider authors should follow in
   102  order to implement providers that can interact with the core Cluster API controllers, as 
   103  documented [here](https://cluster-api.sigs.k8s.io/developer/guide.html) and [here](https://cluster-api.sigs.k8s.io/clusterctl/provider-contract.html).
   104  
   105  By extension, the Cluster API contract includes all the util methods that Cluster API exposes for
   106  making the development of providers simpler and consistent (e.g. everything under `/util` or in  `/test/framework`);
   107  documentation of the utility is available [here](https://pkg.go.dev/sigs.k8s.io/cluster-api?tab=subdirectories).
   108  
   109  The Cluster API contract is linked to the version of the API (e.g. v1beta1 Contract), and it is expected to
   110  provide the same set of guarantees in terms of support window, stability, and upgradability. 
   111  
   112  This makes any change that can impact the Cluster API contract critical and usually:
   113  
   114  - Breaking/major contract changes should go through the CAEP process and be strictly synchronized with the major
   115    release cadence.
   116  - Non-breaking/minor changes can go in minor releases; non-breaking changes are generally:
   117    - Additive in nature
   118    - Default to pre-existing behavior
   119    - Optional as part of the API contract
   120  
   121  ### Logging
   122  
   123  While developing controllers in Cluster API a key requirement is to add logging to observe the system and
   124  to help troubleshooting issues.
   125  
   126  - For CAPI controllers see [Cluster API logging conventions](https://cluster-api.sigs.k8s.io/developer/logging.html).
   127  - For clusterctl see [clusterctl logging conventions](https://github.com/kubernetes-sigs/cluster-api/blob/main/cmd/clusterctl/log/doc.go).
   128  
   129  ### Testing
   130  
   131  Testing plays a crucial role in ensuring the long term maintainability of the project.
   132  
   133  In Cluster API we are committed to have a good test coverage and also to have a nice and consistent style in implementing
   134  tests. For more information see [testing Cluster API](https://cluster-api.sigs.k8s.io/developer/testing.html).