From 6ad6523e8fef317f215c292f6d57d232e9a3f39a Mon Sep 17 00:00:00 2001 From: Jaideep Rao Date: Thu, 18 Jan 2024 00:55:55 -0500 Subject: [PATCH] code-refactorign: update coding practices guide (#1172) * fix: don't set phase to available during host reconciliation (#918) * upgrade golangci-lint Signed-off-by: Jaideep Rao * fix phase reconciliation during host reconciliation Signed-off-by: Jaideep Rao * address review comment Signed-off-by: Jaideep Rao * set phase to pending if ingress not found Signed-off-by: Jaideep Rao --------- Signed-off-by: Jaideep Rao * update owners file (#953) * Move to only adding two roles for managed namespaces (#954) * Move to only adding two roles for managed namespaces --------- Signed-off-by: Salem Elrahal Co-authored-by: Salem Elrahal * feat: expose operator metrics (#928) Track and expose custom operator performance metrics --------- Signed-off-by: Jaideep Rao * add build.os config for readthedocs (#967) * setup 0.8.0 (#966) * feat: Add conversion webhook for ArgoCD v1alpha1 to v1beta1 migration (#964) * Add ArgoCD v1beta1 & deprecate v1alpha1 - Add new ArgoCD v1beta1 api - Mark ArgoCD v1alpha1 as deprecated & add back the removed sso fields - Use server side validation for "kubectl apply" as client side results into failure due to exceeding annotation size limit. Signed-off-by: Siddhesh Ghadi Add funcs for ArgoCD alpha to beta conversion Signed-off-by: Siddhesh Ghadi Add conversion webhook - Create webhook & setup webhook server on 9443 - Disable operator namespaced install via OLM so that OLM can handle certs for webhook server - For manual install, user needs to explicitly configure cert manager to inject certs and enable webhook server in operator by setting env ENABLE_CONVERSION_WEBHOOK="true" Signed-off-by: Siddhesh Ghadi Resolve local build issues Signed-off-by: Siddhesh Ghadi Tweak webhook configs Signed-off-by: Siddhesh Ghadi Update operator installation docs Signed-off-by: Siddhesh Ghadi Add e2e tests Signed-off-by: Siddhesh Ghadi Minor updates Signed-off-by: Siddhesh Ghadi Fix go-lint ci failure Signed-off-by: Siddhesh Ghadi Update docs Signed-off-by: Siddhesh Ghadi Remove webhook from 0.7.0 bundle Signed-off-by: Siddhesh Ghadi Add spaces in bundle Signed-off-by: Siddhesh Ghadi * update 0.8.0 bundle Signed-off-by: Siddhesh Ghadi --------- Signed-off-by: Siddhesh Ghadi * chore(deps): bump pygments from 2.7.4 to 2.15.0 in /docs (#950) Bumps [pygments](https://github.com/pygments/pygments) from 2.7.4 to 2.15.0. - [Release notes](https://github.com/pygments/pygments/releases) - [Changelog](https://github.com/pygments/pygments/blob/master/CHANGES) - [Commits](https://github.com/pygments/pygments/compare/2.7.4...2.15.0) --- updated-dependencies: - dependency-name: pygments dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat: upgrade RH-SSO from 7.5 to 7.6 (#977) * upgrade RH-SSO from 7.5 to 7.6 Signed-off-by: iam-veeramalla * fix: failing tests Signed-off-by: iam-veeramalla * fix: failing tests Signed-off-by: iam-veeramalla --------- Signed-off-by: iam-veeramalla * refactor: Remove dead code (#979) * Remove dead code Signed-off-by: Siddhesh Ghadi * Fix import Signed-off-by: Siddhesh Ghadi * Fix imports Signed-off-by: Siddhesh Ghadi --------- Signed-off-by: Siddhesh Ghadi * Replace ArgoCD v1alpha1 references with v1beta1 (#975) Signed-off-by: Siddhesh Ghadi * refactor: Remove deprecated .spec.resourceCustomizations (#973) * Remove .spec.resourceCustomizations code Signed-off-by: Siddhesh Ghadi * Update docs Signed-off-by: Siddhesh Ghadi * Update docs Signed-off-by: Siddhesh Ghadi * Address review comments Signed-off-by: Siddhesh Ghadi * Fix typo Signed-off-by: Siddhesh Ghadi --------- Signed-off-by: Siddhesh Ghadi * upgrade ArgoCD version to 2.8.2 and update the CRDs (#984) * upgrade ArgoCD version to 2.8.2 and update the CRDs Signed-off-by: ishitasequeira * Update argocd image Signed-off-by: ishitasequeira --------- Signed-off-by: ishitasequeira * chore: Update ArgoCD v1alpha1 deprecation message (#988) * Update ArgoCD v1alpha1 deprecation message Signed-off-by: Siddhesh Ghadi * Run code gen Signed-off-by: Siddhesh Ghadi --------- Signed-off-by: Siddhesh Ghadi * Add support for tls self signed certs in AppSet Gitlab SCM Provider (#985) * add support for tls self signed certs in AppSet Gitlab SCM Provider Signed-off-by: ishitasequeira * add e2e test Signed-off-by: ishitasequeira * add unit tests Signed-off-by: ishitasequeira * renamed field ScmRootCaPath to SCMRootCaPath Signed-off-by: ishitasequeira * Add documentation and address comments Signed-off-by: ishitasequeira * Address comments Signed-off-by: ishitasequeira --------- Signed-off-by: ishitasequeira * chore(deps): bump github.com/argoproj/argo-cd/v2 from 2.8.2 to 2.8.3 (#992) Bumps [github.com/argoproj/argo-cd/v2](https://github.com/argoproj/argo-cd) from 2.8.2 to 2.8.3. - [Release notes](https://github.com/argoproj/argo-cd/releases) - [Changelog](https://github.com/argoproj/argo-cd/blob/master/CHANGELOG.md) - [Commits](https://github.com/argoproj/argo-cd/compare/v2.8.2...v2.8.3) --- updated-dependencies: - dependency-name: github.com/argoproj/argo-cd/v2 dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat: pick up argo cd v2.8.3 (#993) Signed-off-by: iam-veeramalla * fix: replace deprecated syntax in kustomization.yaml (#1000) Signed-off-by: minchao * Missing syntax-highlighting, toggle button for screen mode in argocd-operator docs (#1002) * Update requirements.txt added markdown= 3.3.7 and markdown-include=0.6.0 Signed-off-by: Surajyadav * Delete docs/assets/extra.css deleted extra.css Signed-off-by: Surajyadav * Update mkdocs.yml added markdown_extension markdown_include with codehighlight and toggle for screen mode Signed-off-by: Surajyadav * Update mkdocs.yml Signed-off-by: Surajyadav --------- Signed-off-by: Surajyadav * fix: keycloak probes failure and intermittent perforamance issues (#1007) * fix: keycloak probes failure results in pod crash Signed-off-by: iam-veeramalla * fix: use latest keycloak image to handle performance issue Signed-off-by: iam-veeramalla --------- Signed-off-by: iam-veeramalla * bug: fix heathcheck subkey generation for resources with no group (#1013) * account for empty group during resource customization config subkey generation --------- Signed-off-by: Jaideep Rao * chore(deps): bump golang.org/x/net from 0.11.0 to 0.17.0 (#1019) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.11.0 to 0.17.0. - [Commits](https://github.com/golang/net/compare/v0.11.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: indirect ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add labelSelector option to filter the ArgoCD instances for reconciliation (#961) * Added labelselector string to map conversion Signed-off-by: Raghavi Shirur * Changed data-type for labelSelector to parse string Signed-off-by: Raghavi Shirur * Added code to reconcile selected ArgoCD instances based on label selector Signed-off-by: Raghavi Shirur * remove comments Signed-off-by: Raghavi Shirur * Updated argoCD label fetch, renamed env var Signed-off-by: Raghavi Shirur * Updated unit test and yaml Signed-off-by: Raghavi Shirur * Updated unit test Signed-off-by: Raghavi Shirur * Fix yaml env ValueFrom field Signed-off-by: Raghavi Shirur * Added comments and labelSelector check in main.go Signed-off-by: Raghavi Shirur * removed label-selector option from manifest Signed-off-by: Raghavi Shirur * updated label-selector format in manifests Signed-off-by: Raghavi Shirur * added label selector logs Signed-off-by: Raghavi Shirur * go mod tidy Signed-off-by: Raghavi Shirur * added e2e tests for label-selector Signed-off-by: Raghavi Shirur * restructured kuttl files and added operator patch file Signed-off-by: Raghavi Shirur * go mod tidy Signed-off-by: Raghavi Shirur * corrected kuttl tests for cm failure Signed-off-by: Raghavi Shirur * Added documentation for Environment Variable ARGOCD_LABEL_SELECTOR Signed-off-by: Raghavi Shirur * cleanup Signed-off-by: Raghavi Shirur * improved unit tests and some minor changes Signed-off-by: Raghavi Shirur * kuttl rerun Signed-off-by: Raghavi Shirur * removed env var Signed-off-by: Raghavi Shirur * misc modifications Signed-off-by: Raghavi Shirur * argocd-operator csv correction Signed-off-by: Raghavi Shirur * fix bundle error Signed-off-by: Raghavi Shirur * fix bundle error Signed-off-by: Raghavi Shirur * fix manifests build Signed-off-by: Raghavi Shirur Signed-off-by: Ishita Sequeira Signed-off-by: Raghavi Shirur * Added more unit test cases Signed-off-by: Raghavi Shirur * rebase Signed-off-by: Raghavi Shirur * removed excess reconcilers Signed-off-by: Raghavi Shirur * minor fix Signed-off-by: Raghavi Shirur * removed extraneous test case and cleaned manager.yaml Signed-off-by: Raghavi Shirur * cleaned manager.yaml Signed-off-by: Raghavi Shirur * fix make bundle issue Signed-off-by: Raghavi Shirur * fix make bundle issue Signed-off-by: Raghavi Shirur --------- Signed-off-by: Raghavi Shirur Signed-off-by: Ishita Sequeira Co-authored-by: ishitasequeira * fix: address CVE-2023-39325 (#1022) *address CVE-2023-39325 - upgrade to golang v1.20.10 - disable http/2 for webhook and metrics server, use http/1.1 by default but make it a configurable flag - upgarde k8s library packages to v0.28.3 - Add new structs for keycloak API that were previously part of the (now deprecated) keycloak-operator repo - upgrade to controller-runtime to v0.16.3 - refactor all unit tests --------- Signed-off-by: Jaideep Rao * Add gcp cherry-pick bot config (#1023) Signed-off-by: Siddhesh Ghadi * Add .github/dependabot.yml to enable auto dependency version updates (#1025) * feat(dex): add optional env field (#1005) * feat(dex): add optional env field Signed-off-by: Robert Deusser * fix: remove non-default configuration Signed-off-by: Robert Deusser * fix: v1alpha1 is deprecated Signed-off-by: Robert Deusser * fix: convert dex spec between api versions Signed-off-by: Robert Deusser * fix: ensure there is no diff in the bundle Signed-off-by: Robert Deusser --------- Signed-off-by: Robert Deusser * fix: replace deprecated AddToScheme with Install, and deprecated SchemeGroupVersion with GroupVersion. (#1066) Signed-off-by: Cheng Fang * allow enabling ArgoCD workloads independently (#1021) * allow enabling ArgoCD core workloads independently Signed-off-by: ishitasequeira * fix lint Signed-off-by: ishitasequeira * check for dependent component urls if dependent components are disabled Signed-off-by: ishitasequeira * fix build Signed-off-by: ishitasequeira * fix make bundle Signed-off-by: ishitasequeira * fix tests Signed-off-by: ishitasequeira * Update flags for each component Signed-off-by: ishitasequeira * Update configuration using remote flag Signed-off-by: ishitasequeira * fix CI Signed-off-by: ishitasequeira * Address comments Signed-off-by: ishitasequeira * Addressed feedback Signed-off-by: ishitasequeira * update conversion webhook Signed-off-by: ishitasequeira * fix make build Signed-off-by: ishitasequeira --------- Signed-off-by: ishitasequeira * chore(deps): bump argoproj/argocd in /build/util (#1080) Bumps argoproj/argocd from `d40da8f` to `644c386`. --- updated-dependencies: - dependency-name: argoproj/argocd dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat: upgrade Argo CD for release v.9.0 (#1082) * feat: upgrade Argo CD for release v.9.0 Signed-off-by: iam-veeramalla * fix: unit test failures Signed-off-by: iam-veeramalla --------- Signed-off-by: iam-veeramalla * fix the logic for applicationset resources reconcilation when spec.applicationset.enabled is false (#1089) * fix the logic for applicationset resources reconciliation when spec.applicationset.enabled is false Signed-off-by: ishitasequeira Signed-off-by: Raghavi Shirur Signed-off-by: ishitasequeira * fix tests Signed-off-by: ishitasequeira Signed-off-by: Raghavi Shirur Signed-off-by: ishitasequeira * delete repo server when repo.enabled is set to false Signed-off-by: ishitasequeira Signed-off-by: Raghavi Shirur Signed-off-by: ishitasequeira * Update status.Phase based on component enabled flag Signed-off-by: ishitasequeira Signed-off-by: Raghavi Shirur Signed-off-by: ishitasequeira * Added kuttl tests Signed-off-by: Raghavi Shirur Signed-off-by: ishitasequeira * Added namespace creation step Signed-off-by: Raghavi Shirur Signed-off-by: ishitasequeira * delete services created for resources Signed-off-by: ishitasequeira * delete server deployment when enabled flag set to false Signed-off-by: ishitasequeira * fix e2e test Signed-off-by: ishitasequeira * fix log message Signed-off-by: ishitasequeira * revert kuttl test timeout Signed-off-by: ishitasequeira * Added test for reverse scenario Signed-off-by: Raghavi Shirur * Dir rename Signed-off-by: Raghavi Shirur * Added e2e test for ha mode Signed-off-by: Raghavi Shirur --------- Signed-off-by: ishitasequeira Signed-off-by: Raghavi Shirur Co-authored-by: Raghavi Shirur * docs: enabling/disabling individual argocd core components (#1098) * Add documentation for enabling/disabling argocd core components Signed-off-by: ishitasequeira * rephrase doc Signed-off-by: ishitasequeira * Address comments Signed-off-by: ishitasequeira --------- Signed-off-by: ishitasequeira * fix: Proper reference to where to find default admin password (#1094) Signed-off-by: ikegentz * adding applicationsets in server rbac policy rule (#1140) Signed-off-by: Mangaal * remove extra argoutils Signed-off-by: Jaideep Rao * update coding practices guide Signed-off-by: Jaideep Rao --------- Signed-off-by: Jaideep Rao Signed-off-by: Salem Elrahal Signed-off-by: Siddhesh Ghadi Signed-off-by: dependabot[bot] Signed-off-by: iam-veeramalla Signed-off-by: ishitasequeira Signed-off-by: minchao Signed-off-by: Surajyadav Signed-off-by: Raghavi Shirur Signed-off-by: Ishita Sequeira Signed-off-by: Robert Deusser Signed-off-by: Cheng Fang Signed-off-by: ikegentz Signed-off-by: Mangaal Co-authored-by: Regina Scott <50851526+reginapizza@users.noreply.github.com> Co-authored-by: Salem Elrahal Co-authored-by: Salem Elrahal Co-authored-by: Siddhesh Ghadi <61187612+svghadi@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Abhishek Veeramalla Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> Co-authored-by: Minchao Co-authored-by: Suraj yadav Co-authored-by: Raghavi Co-authored-by: ishitasequeira Co-authored-by: Cheng Fang Co-authored-by: Robert Deusser <5935071+rdeusser@users.noreply.github.com> Co-authored-by: Isaac Gentz Co-authored-by: Mangaal <44372157+Mangaal@users.noreply.github.com> --- coding-standards-and-best-practices.md | 130 ++++++++++++++----------- 1 file changed, 73 insertions(+), 57 deletions(-) diff --git a/coding-standards-and-best-practices.md b/coding-standards-and-best-practices.md index cea3931b8..6111d4f56 100644 --- a/coding-standards-and-best-practices.md +++ b/coding-standards-and-best-practices.md @@ -25,9 +25,20 @@ - If a function needs to be accessed from outside the package (i.e by other controllers etc) then it should accept parameters, and not specify a receiver. This allows anyone with the correct parameters to access this function without needing an instance of that controller's reconciler. Functions like `GetClusterSecrets` in the secret controller, are good candidates for such public facing functions and should accept parameters. +# Helper functions + +- `controllers/argocd/argocdcommon` is a package containing generic functions that need to be accessed across all controllers. These functions are NOT utility functions that belong to either `pkg/util` or `pkg/argoutil` +packages. These functions form a core part of the operator's logic. They have only been isolated to this package so they can be written once and accessed by whoever needs to. Examples of such functions are: + - `UpdateIfChanged` to check and update any resources that have drifted + - `GetContainerImage` to get container images for any component based on first checking the CR, then env vars and then resorting to defaults + - `TriggerRollout` for deployments and statefulsets etc + +Use this package to store core logic function that need to be accessed by all components + # Utility functions - Utility functions are generic go functions that perform low level tasks that are not project specific. These functions likely deal with go data structures or perform generic functions that can be used in any go project. +- Utility functions should not be aware of kubernetes or Argo CD in anyway - Some common examples are string manipulation, merging 2 maps etc. - All utility functions should not be dumped into a single `util.go` file. Each utility function will have a specific domain that it belongs to, and utility functions of the same domain should be grouped together into a single file that is representative of this domain, and placed in the existing `util` package. Examples of this are: @@ -36,6 +47,10 @@ that is representative of this domain, and placed in the existing `util` package - `map.go` containing map manipulation functions - When adding a new function to `util` package, check if this function fits into any of the existing files. If not, create a new file that will contain this and similar functions moving forward. The goal is to have a flat file structure that has files that group similar utility functions together by domain - Main rule of thumb to keep in mind when deciding if a given function should be considered a 'utility' function or not is - Does this function care about Argo CD or Argo CD Operator in any way or not. If the answer is yes, it should likely not be in the utility package, as it is operator specific. +- If there are helper functions that are kubernetes/argo-cd/operator specific, put them under the `pkg/argoutil` package. Examples of what should be in this package are: + - `client.go` - A file to compose clients to talk to different API groups + - `log.go` - For Argo CD specific logging levels + - `auth.go` - For Argo CD authentication helper functions # Constants @@ -78,80 +93,62 @@ Follow general golang conventions when it comes to naming your files. Some of th - filenames can (and should) repeat for files dealing with similar responsibilities across different packages, so that a predictable and intuitive file structure is maintained. +# Error handling -# Constants +- Each error should either be logged or returned. Rarely ever both +- If handling an error in a function that is performing a specific action, annotate the error with context and the name of the function for tracability. eg: -All existing constants can be broken down into the following categories and sub-categories: -- defaults: constants that define fall back values and only get activated if user has not explicitly set a value for a given field - - No specific sub categories; defaults can be grouped by function - image/resource constraints etc -- env var: constants that store environment variable values - - No specific sub categories -- keys: constants that are used as keys as part of key-value pairs in various maps across the operator - - General ArgoCD Keys: Keys that are used for Argo CD wide settings - - Component specific Keys: Keys used in constants that are specific to a given Argo CD component - - Domain specific keys: Keys that are mostly used in labels/annotations, and typically follow a `group.domain/label-name` format. We can group all such constants - together for ease of access -- names: constants that represent fixed names for resources - - names: constants that represent fixed names for resources - - suffixes: constants that represent suffixes used to make unique/ component specific resources -- values: constants that represent standardized field values in maps and other structs, as well as states - - no specific sub categories +``` + if err != nil { + errors.Wrapf(err, "reconcileTLSSecret: failed to calculate checksum for %s in namespace %s", common.ArgoCDRedisServerTLSSecretName, rr.Instance.Namespace) + } +``` + +- If handling an error in a function that just delegates to other functions, no need to add context, as the ground level fn would already have added the required context. Eg: -This structure of constants can be applied to a project wide level, as well as individual component levels. For the project level these constants are defined in separate files in the `common` folder. For individual components they can all be stored in the same `constants.go` file, separeated out into these sections. +``` + ... -## Naming convention + // reconcile ha role + if err := rr.reconcileHARole(); err != nil { + return err + } -- General Argo CD constant names should start with "ArgoCD.." eg: `ArgoCDDefaultLogLevel` -- Component specific constants should start with the component name, e.g: `GrafanaDefaultVersion` -- Domain specific Keys should be named to represent the key itself. For eg: - - `app.kubernetes.io/name` should be stored in a const named `AppK8sKeyName` - - `argocd.argoproj.io/test` => `ArgoCDArgoprojKeyTest` -- Environment variable constants should end in `EnvVar`. Eg: `DexImageEnvVar` + // reconcile serviceaccount + if err := rr.reconcileServiceAccount(); err != nil { + return err + } -General rule of thumb is to make constant names as descriptive as possible (without making them too long) so that it easy to understand what it represents without needing to go to its definition -# Error handling + ... +``` - If a function is reconciling a single resource in a single namespace, return errors immediately, as that resource is critical to the component ``` if err = permissions.CreateClusterRole(desiredClusterRole, *acr.Client); err != nil { - acr.Logger.Error(err, "reconcileClusterRole: failed to create clusterRole") return err } ``` -- If a function is reconciling many instances of a resource across multiple namespaces, or if currently running in a loop, store error but don't break the loop. Return the latest error at the end of the loop (If there are multiple errors, user must solve them one by one from latest to oldest) +- If a function generates a list of independant errors, use the custom defined `util.MultiError` type. Examples for such situations are : + - A function calls multiple other functions that are independently operating, and each is capable of returning errors. In such cases we should collect errors from each function without blocking subsequent functions by returning at first encountered error + - A function may run a loop to perform independent actions. In this case as well we want to report all errors back without blocking reconciliation on other resources ``` - var mutationErr error - ... - if len(request.Mutations) > 0 { - for _, mutation := range request.Mutations { - err := mutation(nil, role, request.Client) - if err != nil { - mutationErr = err - } - } - if mutationErr != nil { - return role, fmt.Errorf("RequestRole: one or more mutation functions could not be applied: %s", mutationErr) - } - } -``` +func (rr *RedisReconciler) reconcileHAConfigMaps() error { + var reconErrs util.MultiError -- If logging the error, then just return error directly -``` -if err = permissions.CreateClusterRole(desiredClusterRole, *acr.Client); err != nil { - acr.Logger.Error(err, "reconcileClusterRole: failed to create clusterRole") - return err - } -``` + err := rr.reconcileHAConfigMap() + reconErrs.Append(err) -- If not logging the error, return error with context for better tracking. If using `fmt.Errorf` use `%w` format specifier for error (see https://stackoverflow.com/questions/61283248/format-errors-in-go-s-v-or-w). e.g: + err = rr.reconcileHAHealthConfigMap() + reconErrs.Append(err) + return reconErrs.ErrOrNil() +} ``` -return role, fmt.Errorf("RequestRole: one or more mutation functions could not be applied: %s", mutationErr) -``` + - At every level decide if encountered error is important enough to return (thereby blocking further execution) : @@ -193,12 +190,30 @@ acr.Logger = ctrl.Log.WithName(ArgoCDApplicationControllerComponent).WithValues( ``` -- Anytime error is encountered, use `Logger.Error` to record error -- Include function name in error message for improved tracking. e.g: +- Log errors ONLY at the component controller's reconciliation level, or for functions that may be called by external packages +- Logging and returning erros at each stage spams the output with the same error message logged over and over as it propogates up the call stack + +``` +if rr.Instance.Spec.HA.Enabled { + // clean up regular redis resources first + if err := rr.DeleteNonHAResources(); err != nil { + rr.Logger.Error(err, "failed to delete non HA redis resources") + } + + // reconcile HA resources + if err := rr.reconcileHA(); err != nil { + rr.Logger.Error(err, "failed to reconcile resources in HA mode") + return err + } + } +``` + +- Include function name in error message for improved tracking ONLY when logging errors. e.g: ``` acr.Logger.Error(err, "reconcileManagedRoles: failed to retrieve role", "name", existingRole.Name, "namespace", existingRole.Namespace) ``` + - Use debug level (`Logger.V(1).Info`) when recording non-essential information. i.e, information on events that don't block happy path execution, but can provide hints if troubleshooting is needed e.g: ``` @@ -206,10 +221,11 @@ acr.Logger.V(1).Info("reconcileManagedRoles: one or more mutations could not be acr.Logger.V(1).Info("reconcileManagedRoles: skip reconciliation in favor of custom role", "name", customRoleName) ``` -- Use Info level (`Logger.Info` or `Logger.V(0).Info`) for all other info-level logs. Any new action taken by the controller that is critical to normal functioning. e.g: +- Use Info level (`Logger.Info` or `Logger.V(0).Info`) for all other info-level logs. Any new action taken by the controller that is critical to normal functioning. +- - No need to mention function names when logging at `info` level. eg: ``` -acr.Logger.V(0).Info("reconcileManagedRoles: role created", "name", desiredRole.Name, "namespace", desiredRole.Namespace) +acr.Logger.V(0).Info("role created", "name", desiredRole.Name, "namespace", desiredRole.Namespace) ``` - Only use log statements to log success/error if the function belongs to a controller package and is invoked by the controller. No need to log statements from utility/helper packages. e.g: @@ -217,7 +233,7 @@ acr.Logger.V(0).Info("reconcileManagedRoles: role created", "name", desiredRole. ``` // app-controller package if err = permissions.CreateRole(desiredRole, *acr.Client); err != nil { - acr.Logger.Error(err, "reconcileManagedRoles: failed to create role", "name", desiredRole.Name, "namespace", desiredRole.Namespace) + acr.Logger.Error(err, "failed to create role", "name", desiredRole.Name, "namespace", desiredRole.Namespace) } // permissions package