Skip to content

Commit

Permalink
code-refactorign: update coding practices guide (argoproj-labs#1172)
Browse files Browse the repository at this point in the history
* fix: don't set phase to available during host reconciliation  (argoproj-labs#918)

* upgrade golangci-lint

Signed-off-by: Jaideep Rao <[email protected]>

* fix phase reconciliation during host reconciliation

Signed-off-by: Jaideep Rao <[email protected]>

* address review comment

Signed-off-by: Jaideep Rao <[email protected]>

* set phase to pending if ingress not found

Signed-off-by: Jaideep Rao <[email protected]>

---------

Signed-off-by: Jaideep Rao <[email protected]>

* update owners file (argoproj-labs#953)

* Move to only adding two roles for managed namespaces (argoproj-labs#954)

* Move to only adding two roles for managed namespaces
---------

Signed-off-by: Salem Elrahal <[email protected]>
Co-authored-by: Salem Elrahal <[email protected]>

* feat: expose operator metrics  (argoproj-labs#928)

Track and expose custom operator performance metrics 
---------

Signed-off-by: Jaideep Rao <[email protected]>

* add build.os config for readthedocs (argoproj-labs#967)

* setup 0.8.0 (argoproj-labs#966)

* feat: Add conversion webhook for ArgoCD v1alpha1 to v1beta1 migration (argoproj-labs#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 <[email protected]>

Add funcs for ArgoCD alpha to beta conversion

Signed-off-by: Siddhesh Ghadi <[email protected]>

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 <[email protected]>

Resolve local build issues

Signed-off-by: Siddhesh Ghadi <[email protected]>

Tweak webhook configs

Signed-off-by: Siddhesh Ghadi <[email protected]>

Update operator installation docs

Signed-off-by: Siddhesh Ghadi <[email protected]>

Add e2e tests

Signed-off-by: Siddhesh Ghadi <[email protected]>

Minor updates

Signed-off-by: Siddhesh Ghadi <[email protected]>

Fix go-lint ci failure

Signed-off-by: Siddhesh Ghadi <[email protected]>

Update docs

Signed-off-by: Siddhesh Ghadi <[email protected]>

Remove webhook from 0.7.0 bundle

Signed-off-by: Siddhesh Ghadi <[email protected]>

Add spaces in bundle

Signed-off-by: Siddhesh Ghadi <[email protected]>

* update 0.8.0 bundle

Signed-off-by: Siddhesh Ghadi <[email protected]>

---------

Signed-off-by: Siddhesh Ghadi <[email protected]>

* chore(deps): bump pygments from 2.7.4 to 2.15.0 in /docs (argoproj-labs#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](pygments/pygments@2.7.4...2.15.0)

---
updated-dependencies:
- dependency-name: pygments
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat: upgrade RH-SSO from 7.5 to 7.6 (argoproj-labs#977)

* upgrade RH-SSO from 7.5 to 7.6

Signed-off-by: iam-veeramalla <[email protected]>

* fix: failing tests

Signed-off-by: iam-veeramalla <[email protected]>

* fix: failing tests

Signed-off-by: iam-veeramalla <[email protected]>

---------

Signed-off-by: iam-veeramalla <[email protected]>

* refactor: Remove dead code (argoproj-labs#979)

* Remove dead code

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Fix import

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Fix imports

Signed-off-by: Siddhesh Ghadi <[email protected]>

---------

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Replace ArgoCD v1alpha1 references with v1beta1 (argoproj-labs#975)

Signed-off-by: Siddhesh Ghadi <[email protected]>

* refactor: Remove deprecated .spec.resourceCustomizations (argoproj-labs#973)

* Remove .spec.resourceCustomizations code

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Update docs

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Update docs

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Address review comments

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Fix typo

Signed-off-by: Siddhesh Ghadi <[email protected]>

---------

Signed-off-by: Siddhesh Ghadi <[email protected]>

* upgrade ArgoCD version to 2.8.2 and update the CRDs (argoproj-labs#984)

* upgrade ArgoCD version to 2.8.2 and update the CRDs

Signed-off-by: ishitasequeira <[email protected]>

* Update argocd image

Signed-off-by: ishitasequeira <[email protected]>

---------

Signed-off-by: ishitasequeira <[email protected]>

* chore: Update ArgoCD v1alpha1 deprecation message (argoproj-labs#988)

* Update ArgoCD v1alpha1 deprecation message

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Run code gen

Signed-off-by: Siddhesh Ghadi <[email protected]>

---------

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Add support for tls self signed certs in AppSet Gitlab SCM Provider (argoproj-labs#985)

* add support for tls self signed certs in AppSet Gitlab SCM Provider

Signed-off-by: ishitasequeira <[email protected]>

* add e2e test

Signed-off-by: ishitasequeira <[email protected]>

* add unit tests

Signed-off-by: ishitasequeira <[email protected]>

* renamed field ScmRootCaPath to SCMRootCaPath

Signed-off-by: ishitasequeira <[email protected]>

* Add documentation and address comments

Signed-off-by: ishitasequeira <[email protected]>

* Address comments

Signed-off-by: ishitasequeira <[email protected]>

---------

Signed-off-by: ishitasequeira <[email protected]>

* chore(deps): bump github.com/argoproj/argo-cd/v2 from 2.8.2 to 2.8.3 (argoproj-labs#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](argoproj/argo-cd@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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat: pick up argo cd v2.8.3 (argoproj-labs#993)

Signed-off-by: iam-veeramalla <[email protected]>

* fix: replace deprecated syntax in kustomization.yaml (argoproj-labs#1000)

Signed-off-by: minchao <[email protected]>

* Missing syntax-highlighting, toggle button for screen mode in argocd-operator docs (argoproj-labs#1002)

* Update requirements.txt

added markdown= 3.3.7 and  markdown-include=0.6.0

Signed-off-by: Surajyadav <[email protected]>

* Delete docs/assets/extra.css

deleted extra.css

Signed-off-by: Surajyadav <[email protected]>

* Update mkdocs.yml

added markdown_extension  markdown_include with  codehighlight and toggle for screen mode

Signed-off-by: Surajyadav <[email protected]>

* Update mkdocs.yml

Signed-off-by: Surajyadav <[email protected]>

---------

Signed-off-by: Surajyadav <[email protected]>

* fix: keycloak probes failure and intermittent perforamance issues (argoproj-labs#1007)

* fix: keycloak probes failure results in pod crash

Signed-off-by: iam-veeramalla <[email protected]>

* fix: use latest keycloak image to handle performance issue

Signed-off-by: iam-veeramalla <[email protected]>

---------

Signed-off-by: iam-veeramalla <[email protected]>

* bug: fix heathcheck subkey generation for resources with no group  (argoproj-labs#1013)

* account for empty group during resource customization config subkey generation

---------

Signed-off-by: Jaideep Rao <[email protected]>

* chore(deps): bump golang.org/x/net from 0.11.0 to 0.17.0 (argoproj-labs#1019)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.11.0 to 0.17.0.
- [Commits](golang/net@v0.11.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add labelSelector option to filter the ArgoCD instances for reconciliation (argoproj-labs#961)

* Added labelselector string to map conversion

Signed-off-by: Raghavi Shirur <[email protected]>

* Changed data-type for labelSelector to parse string

Signed-off-by: Raghavi Shirur <[email protected]>

* Added code to reconcile selected ArgoCD instances based on label selector

Signed-off-by: Raghavi Shirur <[email protected]>

* remove comments

Signed-off-by: Raghavi Shirur <[email protected]>

* Updated argoCD label fetch, renamed env var

Signed-off-by: Raghavi Shirur <[email protected]>

* Updated unit test and yaml

Signed-off-by: Raghavi Shirur <[email protected]>

* Updated unit test

Signed-off-by: Raghavi Shirur <[email protected]>

* Fix yaml env ValueFrom field

Signed-off-by: Raghavi Shirur <[email protected]>

* Added comments and labelSelector check in main.go

Signed-off-by: Raghavi Shirur <[email protected]>

* removed label-selector option from manifest

Signed-off-by: Raghavi Shirur <[email protected]>

* updated label-selector format in manifests

Signed-off-by: Raghavi Shirur <[email protected]>

* added label selector logs

Signed-off-by: Raghavi Shirur <[email protected]>

* go mod tidy

Signed-off-by: Raghavi Shirur <[email protected]>

* added e2e tests for label-selector

Signed-off-by: Raghavi Shirur <[email protected]>

* restructured kuttl files and added operator patch file

Signed-off-by: Raghavi Shirur <[email protected]>

* go mod tidy

Signed-off-by: Raghavi Shirur <[email protected]>

* corrected kuttl tests for cm failure

Signed-off-by: Raghavi Shirur <[email protected]>

* Added documentation for Environment Variable ARGOCD_LABEL_SELECTOR

Signed-off-by: Raghavi Shirur <[email protected]>

* cleanup

Signed-off-by: Raghavi Shirur <[email protected]>

* improved unit tests and some minor changes

Signed-off-by: Raghavi Shirur <[email protected]>

* kuttl rerun

Signed-off-by: Raghavi Shirur <[email protected]>

* removed env var

Signed-off-by: Raghavi Shirur <[email protected]>

* misc modifications

Signed-off-by: Raghavi Shirur <[email protected]>

* argocd-operator csv correction

Signed-off-by: Raghavi Shirur <[email protected]>

* fix bundle error

Signed-off-by: Raghavi Shirur <[email protected]>

* fix bundle error

Signed-off-by: Raghavi Shirur <[email protected]>

* fix manifests build

Signed-off-by: Raghavi Shirur <[email protected]>
Signed-off-by: Ishita Sequeira <[email protected]>
Signed-off-by: Raghavi Shirur <[email protected]>

* Added more unit test cases

Signed-off-by: Raghavi Shirur <[email protected]>

* rebase

Signed-off-by: Raghavi Shirur <[email protected]>

* removed excess reconcilers

Signed-off-by: Raghavi Shirur <[email protected]>

* minor fix

Signed-off-by: Raghavi Shirur <[email protected]>

* removed extraneous test case and cleaned manager.yaml

Signed-off-by: Raghavi Shirur <[email protected]>

* cleaned manager.yaml

Signed-off-by: Raghavi Shirur <[email protected]>

* fix make bundle issue

Signed-off-by: Raghavi Shirur <[email protected]>

* fix make bundle issue

Signed-off-by: Raghavi Shirur <[email protected]>

---------

Signed-off-by: Raghavi Shirur <[email protected]>
Signed-off-by: Ishita Sequeira <[email protected]>
Co-authored-by: ishitasequeira <[email protected]>

* fix: address CVE-2023-39325 (argoproj-labs#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 <[email protected]>

* Add gcp cherry-pick bot config (argoproj-labs#1023)

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Add .github/dependabot.yml to enable auto dependency version updates (argoproj-labs#1025)

* feat(dex): add optional env field (argoproj-labs#1005)

* feat(dex): add optional env field

Signed-off-by: Robert Deusser <[email protected]>

* fix: remove non-default configuration

Signed-off-by: Robert Deusser <[email protected]>

* fix: v1alpha1 is deprecated

Signed-off-by: Robert Deusser <[email protected]>

* fix: convert dex spec between api versions

Signed-off-by: Robert Deusser <[email protected]>

* fix: ensure there is no diff in the bundle

Signed-off-by: Robert Deusser <[email protected]>

---------

Signed-off-by: Robert Deusser <[email protected]>

* fix: replace deprecated AddToScheme with Install, and deprecated SchemeGroupVersion with GroupVersion. (argoproj-labs#1066)

Signed-off-by: Cheng Fang <[email protected]>

* allow enabling ArgoCD workloads independently (argoproj-labs#1021)

* allow enabling ArgoCD core workloads independently

Signed-off-by: ishitasequeira <[email protected]>

* fix lint

Signed-off-by: ishitasequeira <[email protected]>

* check for dependent component urls if dependent components are disabled

Signed-off-by: ishitasequeira <[email protected]>

* fix build

Signed-off-by: ishitasequeira <[email protected]>

* fix make bundle

Signed-off-by: ishitasequeira <[email protected]>

* fix tests

Signed-off-by: ishitasequeira <[email protected]>

* Update flags for each component

Signed-off-by: ishitasequeira <[email protected]>

* Update configuration using remote flag

Signed-off-by: ishitasequeira <[email protected]>

* fix CI

Signed-off-by: ishitasequeira <[email protected]>

* Address comments

Signed-off-by: ishitasequeira <[email protected]>

* Addressed feedback

Signed-off-by: ishitasequeira <[email protected]>

* update conversion webhook

Signed-off-by: ishitasequeira <[email protected]>

* fix make build

Signed-off-by: ishitasequeira <[email protected]>

---------

Signed-off-by: ishitasequeira <[email protected]>

* chore(deps): bump argoproj/argocd in /build/util (argoproj-labs#1080)

Bumps argoproj/argocd from `d40da8f` to `644c386`.

---
updated-dependencies:
- dependency-name: argoproj/argocd
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat: upgrade Argo CD for release v.9.0 (argoproj-labs#1082)

* feat: upgrade Argo CD for release v.9.0

Signed-off-by: iam-veeramalla <[email protected]>

* fix: unit test failures

Signed-off-by: iam-veeramalla <[email protected]>

---------

Signed-off-by: iam-veeramalla <[email protected]>

* fix the logic for applicationset resources reconcilation when spec.applicationset.enabled is false (argoproj-labs#1089)

* fix the logic for applicationset resources reconciliation when spec.applicationset.enabled is false

Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: Raghavi Shirur <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>

* fix tests

Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: Raghavi Shirur <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>

* delete repo server when repo.enabled is set to false

Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: Raghavi Shirur <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>

* Update status.Phase based on component enabled flag

Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: Raghavi Shirur <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>

* Added kuttl tests

Signed-off-by: Raghavi Shirur <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>

* Added namespace creation step

Signed-off-by: Raghavi Shirur <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>

* delete services created for resources

Signed-off-by: ishitasequeira <[email protected]>

* delete server deployment when enabled flag set to false

Signed-off-by: ishitasequeira <[email protected]>

* fix e2e test

Signed-off-by: ishitasequeira <[email protected]>

* fix log message

Signed-off-by: ishitasequeira <[email protected]>

* revert kuttl test timeout

Signed-off-by: ishitasequeira <[email protected]>

* Added test for reverse scenario

Signed-off-by: Raghavi Shirur <[email protected]>

* Dir rename

Signed-off-by: Raghavi Shirur <[email protected]>

* Added e2e test for ha mode

Signed-off-by: Raghavi Shirur <[email protected]>

---------

Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: Raghavi Shirur <[email protected]>
Co-authored-by: Raghavi Shirur <[email protected]>

* docs: enabling/disabling individual argocd core components (argoproj-labs#1098)

* Add documentation for enabling/disabling argocd core components

Signed-off-by: ishitasequeira <[email protected]>

* rephrase doc

Signed-off-by: ishitasequeira <[email protected]>

* Address comments

Signed-off-by: ishitasequeira <[email protected]>

---------

Signed-off-by: ishitasequeira <[email protected]>

* fix: Proper reference to where to find default admin password (argoproj-labs#1094)

Signed-off-by: ikegentz <[email protected]>

* adding applicationsets in server rbac policy rule (argoproj-labs#1140)

Signed-off-by: Mangaal <[email protected]>

* remove extra argoutils

Signed-off-by: Jaideep Rao <[email protected]>

* update coding practices guide

Signed-off-by: Jaideep Rao <[email protected]>

---------

Signed-off-by: Jaideep Rao <[email protected]>
Signed-off-by: Salem Elrahal <[email protected]>
Signed-off-by: Siddhesh Ghadi <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: iam-veeramalla <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: minchao <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Raghavi Shirur <[email protected]>
Signed-off-by: Ishita Sequeira <[email protected]>
Signed-off-by: Robert Deusser <[email protected]>
Signed-off-by: Cheng Fang <[email protected]>
Signed-off-by: ikegentz <[email protected]>
Signed-off-by: Mangaal <[email protected]>
Co-authored-by: Regina Scott <[email protected]>
Co-authored-by: Salem Elrahal <[email protected]>
Co-authored-by: Salem Elrahal <[email protected]>
Co-authored-by: Siddhesh Ghadi <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Abhishek Veeramalla <[email protected]>
Co-authored-by: Ishita Sequeira <[email protected]>
Co-authored-by: Minchao <[email protected]>
Co-authored-by: Suraj yadav <[email protected]>
Co-authored-by: Raghavi <[email protected]>
Co-authored-by: ishitasequeira <[email protected]>
Co-authored-by: Cheng Fang <[email protected]>
Co-authored-by: Robert Deusser <[email protected]>
Co-authored-by: Isaac Gentz <[email protected]>
Co-authored-by: Mangaal <[email protected]>
  • Loading branch information
16 people authored and Julia Teslia committed Apr 24, 2024
1 parent 4a031da commit 6ad6523
Showing 1 changed file with 73 additions and 57 deletions.
130 changes: 73 additions & 57 deletions coding-standards-and-best-practices.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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

Expand Down Expand Up @@ -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) :

Expand Down Expand Up @@ -193,31 +190,50 @@ 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:

```
acr.Logger.V(1).Info("reconcileManagedRoles: one or more mutations could not be applied")
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:

```
// 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
Expand Down

0 comments on commit 6ad6523

Please sign in to comment.