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).