fix: standardize logFormat field naming across ArgoCD components#2108
Open
tzprograms wants to merge 6 commits intoargoproj-labs:masterfrom
Open
fix: standardize logFormat field naming across ArgoCD components#2108tzprograms wants to merge 6 commits intoargoproj-labs:masterfrom
tzprograms wants to merge 6 commits intoargoproj-labs:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStandardizes JSON tag casing for LogFormat (adds deprecated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/argocd_types.go`:
- Line 352: The serialized key was changed to `logFormat` but existing persisted
payloads may use the legacy `logformat`, so add a backward-compatible decode
fallback: keep the `LogFormat string `json:"logFormat,omitempty"` field as
canonical in the struct (argocd_types.go) and implement a custom
UnmarshalJSON/UnmarshalYAML on the Notifications type (the struct that contains
`LogFormat`) that first attempts normal decoding and, if empty, also checks for
the legacy `logformat` key and uses its value; ensure this preserves current
defaulting behavior used by controllers/argocd/notifications.go
(cr.Spec.Notifications.LogFormat) and controllers/argocd/util.go.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa483d5f-92a3-4af4-a727-fcbf489b324e
📒 Files selected for processing (8)
api/v1alpha1/argocd_types.goapi/v1beta1/argocd_types.gobundle/manifests/argocd-operator.clusterserviceversion.yamlbundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yamlcontrollers/argocd/applicationset_test.godeploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yamldeploy/olm-catalog/argocd-operator/0.18.0/argoproj.io_argocds.yaml
tzprograms
force-pushed
the
fix-logformat-consistency
branch
from
d592596 to
29a2864
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bundle/manifests/argocd-operator.clusterserviceversion.yaml (2)
1734-1910:⚠️ Potential issue | 🔴 CriticalRestore
clustertrustbundlesRBAC required by the controller.
certificates.k8s.io/clustertrustbundles(get,list,watch) permission is missing inclusterPermissions, but the controller declares and uses this watch path. This can cause forbidden errors and broken reconciliation for system CA trust updates.Suggested correction
- rules: + - apiGroups: + - certificates.k8s.io + resources: + - clustertrustbundles + verbs: + - get + - list + - watch - apiGroups: - "" resources:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundle/manifests/argocd-operator.clusterserviceversion.yaml` around lines 1734 - 1910, The clusterPermissions block for serviceAccountName argocd-operator-controller-manager is missing RBAC for certificates.k8s.io/clustertrustbundles; add a rule in the clusterPermissions rules array granting apiGroups: ["certificates.k8s.io"], resources: ["clustertrustbundles"], verbs: ["get","list","watch"] (so the controller's watch on clustertrustbundles succeeds and avoids forbidden errors).
427-892:⚠️ Potential issue | 🟡 MinorRemove accidental trailing apostrophes in
specDescriptors.displayName.A large set of display names now include a trailing
'(for example,Ingress Enabled'). This is a user-facing metadata regression in CSV descriptors.Suggested correction (example)
- displayName: Ingress Enabled' + displayName: Ingress Enabled - displayName: Resource Requirements' + displayName: Resource RequirementsAlso applies to: 1055-1479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundle/manifests/argocd-operator.clusterserviceversion.yaml` around lines 427 - 892, Many specDescriptors have accidental trailing apostrophes in their displayName values (e.g., "Ingress Enabled'") causing a user-facing metadata regression; locate all instances of the displayName field under specDescriptors (reference: specDescriptors.displayName / displayName entries like Ingress Enabled', Route Enabled', Config Management Plugins', Resource Requirements', etc.) and remove the trailing single-quote character from each displayName string across the CSV YAML (also apply fixes in the other affected range noted: lines ~1055-1479) so all displayName values are plain, correctly quoted strings without the extra apostrophe.
🧹 Nitpick comments (1)
deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml (1)
655-683: Description text changes appear unrelated to PR scope.Several description fields have been modified (e.g., Prometheus-related fields), but these changes are not mentioned in the PR objectives which focus on
logFormatfield naming standardization. If these are intentional refinements, consider documenting them in the PR description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml` around lines 655 - 683, The PR includes unrelated description edits in the CSV (e.g., changes to displayName "Ingress Enabled'" and "Route Enabled'", and Prometheus description blocks tied to paths prometheus.ingress.enabled, prometheus.route.enabled, prometheus.host, prometheus.enabled) that don’t match the stated scope (logFormat renaming); revert those description/displayName changes to their original values or remove them from this commit and create a separate PR with clear documentation if they are intentional, and update the PR description to list any intentional CSV text changes so reviewers are aware.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bundle/manifests/argocd-operator.clusterserviceversion.yaml`:
- Around line 266-267: The CSV metadata was accidentally downgraded—reset any
changes that set "name: argocd-operator.v0.18.0" and "replaces: v0.16.0" (and
any corresponding image/tag fields) back to the original/current release values
prior to this PR; ensure you update the CSV's name, spec.replaces and related
image/version tags to match the existing release stream rather than rolling back
to v0.18.0/v0.16.0 so the upgrade path remains unchanged.
In
`@deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml`:
- Line 427: Several displayName fields in the argocd operator CSVs have an
extraneous trailing apostrophe (e.g., "Application Instance Label Key'"); locate
all displayName keys in argocd-operator.v0.18.0.clusterserviceversion.yaml and
the bundle/manifests/argocd-operator.clusterserviceversion.yaml (both v1alpha1
and v1beta1 spec descriptors) and remove the trailing single quote from each
value so they read correctly (e.g., "Application Instance Label Key"); ensure
you update every affected displayName occurrence (there are 79 total) and keep
YAML quoting/escaping intact.
---
Outside diff comments:
In `@bundle/manifests/argocd-operator.clusterserviceversion.yaml`:
- Around line 1734-1910: The clusterPermissions block for serviceAccountName
argocd-operator-controller-manager is missing RBAC for
certificates.k8s.io/clustertrustbundles; add a rule in the clusterPermissions
rules array granting apiGroups: ["certificates.k8s.io"], resources:
["clustertrustbundles"], verbs: ["get","list","watch"] (so the controller's
watch on clustertrustbundles succeeds and avoids forbidden errors).
- Around line 427-892: Many specDescriptors have accidental trailing apostrophes
in their displayName values (e.g., "Ingress Enabled'") causing a user-facing
metadata regression; locate all instances of the displayName field under
specDescriptors (reference: specDescriptors.displayName / displayName entries
like Ingress Enabled', Route Enabled', Config Management Plugins', Resource
Requirements', etc.) and remove the trailing single-quote character from each
displayName string across the CSV YAML (also apply fixes in the other affected
range noted: lines ~1055-1479) so all displayName values are plain, correctly
quoted strings without the extra apostrophe.
---
Nitpick comments:
In
`@deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml`:
- Around line 655-683: The PR includes unrelated description edits in the CSV
(e.g., changes to displayName "Ingress Enabled'" and "Route Enabled'", and
Prometheus description blocks tied to paths prometheus.ingress.enabled,
prometheus.route.enabled, prometheus.host, prometheus.enabled) that don’t match
the stated scope (logFormat renaming); revert those description/displayName
changes to their original values or remove them from this commit and create a
separate PR with clear documentation if they are intentional, and update the PR
description to list any intentional CSV text changes so reviewers are aware.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4404616b-13dc-4f48-9e84-80980701c8ab
📒 Files selected for processing (8)
api/v1alpha1/argocd_types.goapi/v1beta1/argocd_types.gobundle/manifests/argocd-operator.clusterserviceversion.yamlbundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yamlcontrollers/argocd/applicationset_test.godeploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yamldeploy/olm-catalog/argocd-operator/0.18.0/argoproj.io_argocds.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- api/v1alpha1/argocd_types.go
- api/v1beta1/argocd_types.go
- controllers/argocd/applicationset_test.go
deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml
Show resolved
Hide resolved
Signed-off-by: Tejas Soham <tejassoham05@gmail.com>
tzprograms
force-pushed
the
fix-logformat-consistency
branch
from
29a2864 to
b97074a
Compare
Signed-off-by: Tejas Soham <tejassoham05@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml (1)
1734-1910:⚠️ Potential issue | 🟠 MajorRe-add
clustertrustbundlesRBAC to CSV cluster permissions.
install.spec.clusterPermissionsis missingcertificates.k8s.ioaccess forclustertrustbundles(get/list/watch). The controller conditionally registers a ClusterTrustBundle watch and maps CTB events forspec.repo.systemCATrust; without this RBAC, that path can fail with forbidden errors when the API is present.🔧 Proposed fix
- rules: @@ - apiGroups: - config.openshift.io resources: - clusterversions verbs: - get - list - watch + - apiGroups: + - certificates.k8s.io + resources: + - clustertrustbundles + verbs: + - get + - list + - watch - apiGroups: - monitoring.coreos.com resources: - prometheuses - prometheusrules🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml` around lines 1734 - 1910, install.spec.clusterPermissions is missing RBAC for certificates.k8s.io clustertrustbundles causing forbidden errors when the controller registers a ClusterTrustBundle watch; add a rule under the existing clusterPermissions entry (the block that ends with serviceAccountName: argocd-operator-controller-manager) that grants apiGroups: ["certificates.k8s.io"] resources: ["clustertrustbundles"] verbs: ["get","list","watch"] so the controller (argocd-operator-controller-manager) can watch and map ClusterTrustBundle events.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1beta1/argocd_types.go`:
- Line 222: Add custom UnmarshalJSON implementations for ArgoCDApplicationSet
and ArgoCDNotifications that preserve backward compatibility for the legacy
lowercase key: in each UnmarshalJSON, unmarshal into a type alias/temporary
struct that has the new camelCase LogFormat field and also captures a legacy
"logformat" json key (e.g., LegacyLogFormat string
`json:"logformat,omitempty"`); after unmarshalling, if the primary LogFormat on
the struct is empty but LegacyLogFormat is set, assign LegacyLogFormat to
LogFormat so existing CRs with spec.applicationSet.logformat or
spec.notifications.logformat are honored; ensure you implement the method
signature func (a *ArgoCDApplicationSet) UnmarshalJSON(data []byte) error and
likewise for ArgoCDNotifications and delegate unknown fields to the original
struct to avoid data loss.
---
Outside diff comments:
In
`@deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml`:
- Around line 1734-1910: install.spec.clusterPermissions is missing RBAC for
certificates.k8s.io clustertrustbundles causing forbidden errors when the
controller registers a ClusterTrustBundle watch; add a rule under the existing
clusterPermissions entry (the block that ends with serviceAccountName:
argocd-operator-controller-manager) that grants apiGroups:
["certificates.k8s.io"] resources: ["clustertrustbundles"] verbs:
["get","list","watch"] so the controller (argocd-operator-controller-manager)
can watch and map ClusterTrustBundle events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6621c9cc-847c-4a43-9351-4b0f20c912f4
📒 Files selected for processing (10)
api/v1alpha1/argocd_types.goapi/v1beta1/argocd_types.gobundle/manifests/argocd-operator.clusterserviceversion.yamlbundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yamlcontrollers/argocd/applicationset_test.godeploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yamldeploy/olm-catalog/argocd-operator/0.18.0/argoproj.io_argocds.yamldeploy/olm-catalog/argocd-operator/0.19.0/argocd-operator.v0.19.0.clusterserviceversion.yamldeploy/olm-catalog/argocd-operator/0.19.0/argoproj.io_argocds.yaml
✅ Files skipped from review due to trivial changes (1)
- deploy/olm-catalog/argocd-operator/0.19.0/argocd-operator.v0.19.0.clusterserviceversion.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- api/v1alpha1/argocd_types.go
- bundle/manifests/argocd-operator.clusterserviceversion.yaml
tzprograms
force-pushed
the
fix-logformat-consistency
branch
from
b97074a to
4fe72f8
Compare
Collaborator
|
Hi @tzprograms , thank you for working on this. We need to take care of a few things to ensure smooth upgrades:
|
Author
|
Thank you for the review @svghadi , I will take a look at them as soon as possible. |
… compatibility Signed-off-by: Tejas Soham <tejassoham05@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/argocd_types.go (1)
163-165:⚠️ Potential issue | 🟡 MinorAdd deprecated
Logformatfield toArgoCDApplicationSetfor backward compatibility consistency.The v1beta1
ArgoCDApplicationSetstruct includes bothLogformat(deprecated) andLogFormatfields, but the v1alpha1 version in this file contains onlyLogFormat. This creates an inconsistency: existing v1alpha1 CRs usingspec.applicationSet.logformatwould fail to deserialize after upgrade, causing configuration loss.Add the deprecated field to match v1beta1:
// LogFormat refers to the log format used by the ApplicationSet component. Defaults to ArgoCDDefaultLogFormat if not configured. Valid options are text or json. // +kubebuilder:validation:Enum=text;json // Deprecated: use LogFormat instead. Logformat string `json:"logformat,omitempty"` LogFormat string `json:"logFormat,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/argocd_types.go` around lines 163 - 165, Add a deprecated `Logformat` field to the ArgoCDApplicationSet struct in api/v1alpha1/argocd_types.go to match v1beta1: declare `Logformat string `json:"logformat,omitempty"`` immediately before the existing `LogFormat string `json:"logFormat,omitempty"``, include the same kubebuilder validation tag (`+kubebuilder:validation:Enum=text;json`) and a deprecation comment (`// Deprecated: use LogFormat instead.`) so v1alpha1 CRs with `spec.applicationSet.logformat` will still deserialize; update the struct definition for ArgoCDApplicationSet accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/argocd_types.go`:
- Around line 350-355: The validation marker and doc comment are attached to the
deprecated Logformat field instead of the canonical LogFormat field; move the
doc comment ("LogFormat refers to... Valid options are text or json.") and the
kubebuilder marker (+kubebuilder:validation:Enum=text;json) from the Logformat
field to the LogFormat field so the CRD validates the new json key, and keep the
deprecated Logformat field with a brief deprecation comment only; update
comments to reference the correct field names (Logformat vs LogFormat) and
ensure the struct tags remain unchanged.
In `@bundle/manifests/argoproj.io_argocds.yaml`:
- Around line 9134-9147: The new preferred field "logFormat" in the
ApplicationSet CRD lost the enum validation and description that "logformat"
(deprecated) still enforces; restore the same validation and docs onto the new
field so both accept only "text" or "json" and share the same description and
default reference (ArgoCDDefaultLogFormat). Update the "logFormat" schema entry
to include the description text, enum ["text","json"], and type string
(mirroring the existing "logformat" block) so deprecation preserves runtime
validation and behavior for ApplicationSet/ArgoCDDefaultLogFormat.
- Around line 2136-2147: The new `logFormat` field currently lacks the enum and
description that exist on the deprecated `logformat`; restore the schema
metadata by adding the same description, enum (text|json) and default/mention of
ArgoCDDefaultLogFormat to `logFormat` in the source type (mirroring what
`logformat` carries) so CRD validation and `oc explain` work as before, then
regenerate the CRD manifests; ensure you update the type that defines
`logFormat` and re-run the codegen that emits
bundle/manifests/argoproj.io_argocds.yaml so both `logFormat` and deprecated
`logformat` remain present with equivalent schema metadata.
In `@config/crd/bases/argoproj.io_argocds.yaml`:
- Around line 9123-9124: Add a descriptive field and enum validation for the
logFormat property in the ApplicationSet CRD: update the logFormat schema (the
logFormat property) to include a descriptive "description" string explaining
accepted formats and an "enum" array listing allowed values (e.g. "json" and
"text") so the CRD validates inputs; ensure the modification mirrors the same
pattern used elsewhere in the repo for other resources (add description and enum
under the logFormat definition) and keep the property type as string.
- Around line 18255-18256: Add a descriptive comment and enum validation for the
logFormat field (currently declared as "logFormat: type: string") similar to the
earlier notifications fix: update the schema for logFormat to include a clear
description and an "enum" list of allowed values and (if applicable) a default,
so the OpenAPI/CRD validation enforces permitted log formats; locate the
logFormat field in the ArgoCD CRD schema and add the description and enum
entries consistent with the other notifications logFormat declaration.
- Around line 2125-2126: The new logFormat field is missing the description and
enum validation present on the deprecated logformat field; update each logFormat
schema definition to mirror the deprecated field by adding a descriptive
"description" string and an "enum" constraint with the allowed values
["text","json"], ensuring the type remains "string" and the validation
message/description matches the deprecated logformat entry; apply this same
change to every occurrence of logFormat in the file so all definitions enforce
the same allowed values and documentation.
In `@controllers/argocd/applicationset.go`:
- Around line 64-71: The notifications controller currently reads
cr.Spec.Notifications.LogFormat directly; implement the same fallback logic used
in the ApplicationSet controller by introducing a local variable (e.g.,
logFormat := cr.Spec.Notifications.LogFormat; if logFormat == "" { logFormat =
cr.Spec.Notifications.Logformat }) and then append "--logformat" and
getLogFormat(logFormat) to cmd so deprecated Logformat is honored for backward
compatibility.
In `@deploy/olm-catalog/argocd-operator/0.19.0/argoproj.io_argocds.yaml`:
- Around line 18266-18267: The Notifications schema's logFormat property lacks
the description and allowed-values enum—update the logFormat field (under the
Notifications section) to include a clear description and the same enum
constraint used elsewhere (e.g., allowed formats like "json" and "text") so it
matches the other occurrences; modify the logFormat definition to add the
description string and the enum array alongside its type to ensure consistency
with other logFormat definitions.
- Around line 9134-9135: The ApplicationSet schema entry for the new field
logFormat lacks the description and enum constraint present on the deprecated
logformat; update the logFormat schema (the logFormat property) to include the
same description text and the same enum values as the deprecated logformat field
so validation and documentation remain consistent with Notifications and the
legacy property.
- Around line 2136-2137: The new logFormat field currently lacks validation and
description; update the schema for logFormat to match the deprecated logformat
field by copying its description and adding the enum constraint ["text","json"]
so only "text" or "json" are accepted, ensuring parity between logFormat and
logformat.
---
Outside diff comments:
In `@api/v1alpha1/argocd_types.go`:
- Around line 163-165: Add a deprecated `Logformat` field to the
ArgoCDApplicationSet struct in api/v1alpha1/argocd_types.go to match v1beta1:
declare `Logformat string `json:"logformat,omitempty"`` immediately before the
existing `LogFormat string `json:"logFormat,omitempty"``, include the same
kubebuilder validation tag (`+kubebuilder:validation:Enum=text;json`) and a
deprecation comment (`// Deprecated: use LogFormat instead.`) so v1alpha1 CRs
with `spec.applicationSet.logformat` will still deserialize; update the struct
definition for ArgoCDApplicationSet accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c498fb01-8fc4-4a35-8305-6a375a501d5d
📒 Files selected for processing (8)
api/v1alpha1/argocd_types.goapi/v1beta1/argocd_types.gobundle/manifests/argocd-operator.clusterserviceversion.yamlbundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yamlcontrollers/argocd/applicationset.godeploy/olm-catalog/argocd-operator/0.19.0/argocd-operator.v0.19.0.clusterserviceversion.yamldeploy/olm-catalog/argocd-operator/0.19.0/argoproj.io_argocds.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- deploy/olm-catalog/argocd-operator/0.19.0/argocd-operator.v0.19.0.clusterserviceversion.yaml
- bundle/manifests/argocd-operator.clusterserviceversion.yaml
- api/v1beta1/argocd_types.go
Signed-off-by: Tejas Soham <tejassoham05@gmail.com>
Signed-off-by: Tejas Soham <tejassoham05@gmail.com>
anandf
reviewed
Mar 17, 2026
| // LogFormat refers to the log format used by the ApplicationSet component. Defaults to ArgoCDDefaultLogFormat if not configured. Valid options are text or json. | ||
| // +kubebuilder:validation:Enum=text;json | ||
| LogFormat string `json:"logformat,omitempty"` | ||
| LogFormat string `json:"logFormat,omitempty"` |
Collaborator
There was a problem hiding this comment.
Can we add a tag for setting the default
// +kubebuilder:validation:Enum=text;json
// +kubebuilder:default:=json
Collaborator
There was a problem hiding this comment.
Maybe we can set a default in the next API version. Doing it in the same version might cause backward compatibility issues, since conversion webhooks can’t handle this within the same API version.
anandf
reviewed
Mar 17, 2026
|
|
||
| // Higher Preference to the new logFormat field. Fall back to the deprecated logformat field for backward compatibility. | ||
| logFormat := cr.Spec.ApplicationSet.LogFormat | ||
| if logFormat == "" { |
Collaborator
There was a problem hiding this comment.
if we set a default value, this may not be empty. To handle backward compatibility, can we use conversion webhook to convert any logformat customer has set to logFormat. If that is not possible, then we can have this code and not set any default value.
Collaborator
There was a problem hiding this comment.
I think we can implement this change in the next CRD version bump, since conversion webhooks cannot handle changes within the same CRD version.
anandf
reviewed
Mar 17, 2026
| // Higher Preference to the new logFormat field. Fall back to the deprecated logformat field for backward compatibility. | ||
| logFormat := cr.Spec.ApplicationSet.LogFormat | ||
| if logFormat == "" { | ||
| //nolint:staticcheck // fallback to deprecated field for backward compatibility |
Collaborator
There was a problem hiding this comment.
Why is nolint required ? does the linter complain about anything ?
svghadi
requested changes
Mar 30, 2026
Collaborator
svghadi
left a comment
svghadi
left a comment
There was a problem hiding this comment.
Can we add some unit and e2e tests?
Signed-off-by: Tejas Soham <tejassoham05@gmail.com>
svghadi
approved these changes
Apr 8, 2026
Collaborator
svghadi
left a comment
svghadi
left a comment
There was a problem hiding this comment.
LGTM. Thanks @tzprograms
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
What does this PR do / Why we need it
This PR fixes an inconsistency in logging configuration field naming across ArgoCD components.
The fields:
.spec.applicationSet.logformat.spec.notifications.logformatwere introduced in lowercase, while other components already use camelCase:
.spec.controller.logFormat.spec.server.logFormat.spec.repo.logFormatThis change standardizes the field naming to
logFormatto maintain consistency across all ArgoCD components.Changes
make manifests)make bundle)Which issue(s) this PR fixes
Fixes #1821
How to test changes
Install the updated CRD.
Verify the schema using:
-
oc explain argocd.spec.applicationSet.logFormat-
oc explain argocd.spec.notifications.logFormatConfirm that lowercase fields no longer exist:
-
oc explain argocd.spec.applicationSet.logformat-
oc explain argocd.spec.notifications.logformatExpected result: lowercase fields should not exist.
Summary by CodeRabbit
Bug Fixes
New Features
Chores