feat(helm): Add Principal component Helm chart#947
Merged
jannfis merged 19 commits intoJun 3, 2026
Conversation
Signed-off-by: navin <navinchandrarai444@gmail.com>
Signed-off-by: navin <navinchandrarai444@gmail.com>
enbiyagoral
requested review from
chetan-rns,
drewbailey,
jannfis,
jgwest,
jparsai and
mikeshng
as code owners
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds a complete Helm chart for argocd-agent-principal: chart metadata and packaging, values/schema, template helpers, Deployment and params ConfigMap, Services (gRPC/metrics/healthz/redis/resource-proxy), RBAC and ServiceAccount templates, NetworkPolicy, ServiceMonitor, README/NOTES, and a Helm-hook test suite. ChangesPrincipal Helm Chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
… NetworkPolicy, ServiceMonitor, and aligned deployment Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
enbiyagoral
force-pushed
the
feat/principal-helm-chart
branch
from
d31dd2e to
337c3a1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #947 +/- ##
=======================================
Coverage 47.65% 47.65%
=======================================
Files 123 123
Lines 18062 18062
=======================================
Hits 8608 8608
Misses 8687 8687
Partials 767 767 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
install/helm-repo/argocd-agent-principal/values.schema.json (2)
559-583: ⚡ Quick win
bitnamilegacy/kubectlis the legacy, no-longer-updated Bitnami image namespace.
bitnamilegacyis explicitly labeled "Legacy Bitnami images (no longer updated)" on Docker Hub. This means the default test image won't receive security patches. For production workloads and long-term support, users are encouraged to adopt Bitnami Secure Images. Even though tests are opt-in, an unmaintained image as the default is a poor starting point. Consider usingbitnami/kubectl(Bitnami Secure Images) or a distroless/chainguard kubectl image instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/values.schema.json` around lines 559 - 583, The tests.image default currently uses the legacy Bitnami namespace; update the tests schema so the "image" property default is a maintained image (e.g., change default from "bitnamilegacy/kubectl" to "bitnami/kubectl" or a distroless/chainguard kubectl image) and adjust the "description" for the "image" property to mention it uses Bitnami Secure Images (or the chosen maintained alternative) to avoid recommending an unmaintained image; locate the "tests" object and the "image" property in the values.schema.json and update its "default" and "description" accordingly.
91-119: ⚡ Quick win
resourcesschema prevents common Kubernetes patterns.The combination of
additionalProperties: falseandrequired: ["cpu", "memory"]on bothlimitsandrequests, plusrequired: ["limits", "requests"]at the top level, means:
- Users must always provide both
limitsandrequests; omittingrequests(Guaranteed QoS or limits-only) will fail schema validation.- Extended resource types (
ephemeral-storage,hugepages-2Mi, NVIDIA GPU, etc.) are blocked.Consider relaxing the schema so overrides remain valid without forcing the full structure:
♻️ Proposed fix
"resources": { - "additionalProperties": false, + "additionalProperties": true, "description": "Resource requests and limits for the principal Pod.", "properties": { "limits": { - "additionalProperties": false, + "additionalProperties": true, "properties": { "cpu": { "title": "cpu", "type": "string" }, "memory": { "title": "memory", "type": "string" } }, - "required": ["cpu", "memory"], "title": "limits", "type": "object" }, "requests": { - "additionalProperties": false, + "additionalProperties": true, "properties": { "cpu": { "title": "cpu", "type": "string" }, "memory": { "title": "memory", "type": "string" } }, - "required": ["cpu", "memory"], "title": "requests", "type": "object" } }, - "required": ["limits", "requests"], "title": "resources", "type": "object" },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/values.schema.json` around lines 91 - 119, The "resources" JSON schema is too strict: it currently forces both "limits" and "requests" and only allows "cpu" and "memory", blocking common Kubernetes patterns and extended resources. Relax the schema by removing the top-level required: ["limits","requests"] and removing the required: ["cpu","memory"] inside both "limits" and "requests", and set "additionalProperties": true for the "limits" and "requests" objects (or remove their additionalProperties:false) so arbitrary resource keys (ephemeral-storage, GPUs, hugepages, etc.) are allowed; keep the existing "cpu" and "memory" property definitions but allow other properties through these changes on the "resources", "limits", and "requests" symbols so users can supply limits-only, requests-omitted, or extended resources.install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml (1)
10-12: ⚡ Quick winUse the configurable test image instead of a hardcoded
busybox.
image: busybox(no tag) is a floatinglatestreference and inconsistent withtest-deployment.yaml, which uses"{{ .Values.tests.image }}:{{ .Values.tests.tag }}". Environments with image pull restrictions or image digests pinned via values will behave differently across the two test pods.🔧 Proposed fix
- - name: wget - image: busybox + - name: busybox + image: "{{ .Values.tests.image }}:{{ .Values.tests.tag }}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml` around lines 10 - 12, Replace the hardcoded image in the test ConfigMap container named "wget" with the same configurable test image used in test-deployment.yaml; specifically change the image field for the "wget" container to use the Helm template values (the .Values.tests.image and .Values.tests.tag) so the test pod honors image/tag overrides and pull policies consistently across tests.install/helm-repo/argocd-agent-principal/templates/principal-resource-proxy-service.yaml (1)
6-14: ⚡ Quick winHardcoded service name and port 9090; inconsistent with the rest of the service templates.
Every other service in this chart drives its name from a helper (e.g.,
argocd-agent-principal.metricsServiceName) and its port from a.Valuesreference. This service hardcodes bothargocd-agent-resource-proxyand9090. At minimum the port should come from a value so operators can override it without patching the template, and the name should use a helper for consistency.🔧 Proposed fix
metadata: namespace: {{ include "argocd-agent-principal.namespace" . }} - name: argocd-agent-resource-proxy + name: {{ include "argocd-agent-principal.resourceProxyServiceName" . }} labels: {{- include "argocd-agent-principal.labels" . | nindent 4 }} spec: ports: - name: resource-proxy protocol: TCP - port: 9090 - targetPort: 9090 + port: {{ .Values.principal.resourceProxy.port }} + targetPort: {{ .Values.principal.resourceProxy.port }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/templates/principal-resource-proxy-service.yaml` around lines 6 - 14, The service template hardcodes the service name "argocd-agent-resource-proxy" and port 9090; change it to use the chart helper and a values entry instead: replace the literal name with a helper call (e.g., use a helper similar to argocd-agent-principal.metricsServiceName — create/use argocd-agent-principal.resourceProxyServiceName) and replace the hardcoded port 9090 with a values reference (e.g., .Values.resourceProxy.port or .Values.service.resourceProxy.port) so operators can override it; update the values.yaml schema with the new port key if missing and ensure templates reference that key consistently.install/helm-repo/argocd-agent-principal/templates/tests/test-labels.yaml (1)
6-7: ⚡ Quick winConsider adding
helm.sh/hook-delete-policyto all test podsWithout a
hook-delete-policyannotation, eachhelm testrun creates a new pod that is never cleaned up, causing them to accumulate in the namespace. Addingbefore-hook-creationkeeps the namespace tidy while still allowing log inspection after a failure.annotations: "helm.sh/hook": test + "helm.sh/hook-delete-policy": before-hook-creationThis applies to all test Pod manifests in
templates/tests/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/templates/tests/test-labels.yaml` around lines 6 - 7, Add the helm.sh/hook-delete-policy: before-hook-creation annotation to the test Pod annotation blocks so Helm will remove previous test pods; locate the annotations map in the test Pod manifests (e.g., the annotations block in test-labels.yaml under templates/tests/) and add the "helm.sh/hook-delete-policy": "before-hook-creation" entry alongside "helm.sh/hook": "test" in each test Pod manifest.install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml (1)
139-155: ⚡ Quick win
kubectl get endpointsis deprecated as of Kubernetes 1.33As of Kubernetes 1.33, the Endpoints API is officially deprecated, and the API server will return warnings to users who read or write Endpoints resources rather than using EndpointSlices. This will generate noise in test pod logs and the API will eventually be removed.
Consider switching to
kubectl get endpointslice -l kubernetes.io/service-name=<svc>and querying.endpoints[0].addresses[0]instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml` around lines 139 - 155, Replace the deprecated kubectl get endpoints calls used to populate METRICS_ENDPOINT_IP and HEALTHZ_ENDPOINT_IP with kubectl get endpointslice queries filtered by the service name label (kubernetes.io/service-name=<svc>) and extract the first endpoint address (use jsonpath .items[0].endpoints[0].addresses[0].ip); update the commands that reference METRICS_SERVICE and HEALTHZ_SERVICE in the test script so they query EndpointSlices in the same namespace (the include "argocd-agent-principal.namespace" template) and assign the result to METRICS_ENDPOINT_IP and HEALTHZ_ENDPOINT_IP respectively, preserving the existing existence checks and exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@install/helm-repo/argocd-agent-principal/templates/principal-deployment.yaml`:
- Around line 354-359: The REDIS_PASSWORD env var is currently required
unconditionally and will cause CreateContainerConfigError if the secret is
missing; update the template so the env var is only added when a secret name is
provided and mark its secretKeyRef as optional. Specifically, wrap the
REDIS_PASSWORD env var block (the entry referencing .Values.redisSecretName and
.Values.redisPasswordKey) in a Helm conditional that checks
.Values.redisSecretName is non-empty (e.g., if .Values.redisSecretName) and/or
add optional: true under the secretKeyRef for REDIS_PASSWORD so Kubernetes will
not fail pod creation when the Secret is absent.
In
`@install/helm-repo/argocd-agent-principal/templates/principal-networkpolicy.yaml`:
- Around line 16-34: The NetworkPolicy ingress rules under the
principal-networkpolicy template use podSelector-only from blocks (controlled by
.Values.networkPolicy.ingress.grpc.enabled and
.Values.principal.redisProxy.enabled) which restricts traffic to the same
namespace; update those rules in principal-networkpolicy.yaml to pair each
podSelector with namespaceSelector: {} so pods with the matching labels from
other namespaces in the same cluster are allowed, and keep the current metrics
rule pattern as reference; also add a brief note in values.yaml docs explaining
that cross-cluster agents require ipBlock entries via
.Values.networkPolicy.ingress.extraRules or disabling the grpc/redis proxy
networkPolicy flags.
In `@install/helm-repo/argocd-agent-principal/templates/principal-sa.yaml`:
- Around line 1-13: The ServiceAccount template omits the
serviceAccount.automountServiceAccountToken value so setting it in values.yaml
has no effect; update the principal-sa.yaml template (around the ServiceAccount
metadata/annotations block) to conditionally render automountServiceAccountToken
when the key exists (use hasKey on .Values.serviceAccount for
"automountServiceAccountToken") and output the boolean literal from
.Values.serviceAccount.automountServiceAccountToken (e.g., inside an {{- if
hasKey .Values.serviceAccount "automountServiceAccountToken" }} ... {{- end }}
block) so both true and false are respected.
In
`@install/helm-repo/argocd-agent-principal/templates/tests/test-deployment.yaml`:
- Around line 55-62: The test currently compares REPLICAS to a hardcoded "1"
which breaks when .Values.replicaCount differs; set an EXPECTED_REPLICAS
variable using the Helm value (e.g. EXPECTED_REPLICAS="{{ .Values.replicaCount |
default 1 }}" in the template) and then compare REPLICAS against
EXPECTED_REPLICAS (use the existing REPLICAS and DEPLOYMENT_NAME variables and
the same namespace include "argocd-agent-principal.namespace") so the test
validates the templated replica count instead of the literal "1".
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-labels.yaml`:
- Around line 126-147: Guard the ClusterRole label checks behind the Helm
conditional that controls creation: wrap the block that runs kubectl get
clusterrole "$CLUSTER_ROLE_NAME" and subsequent checks that reference
CLUSTER_ROLE_LABELS with the same template guard used for creation (check
.Values.rbac.createClusterRole), so the kubectl call and grep assertions only
run when rbac.createClusterRole is true; ensure you reference the existing
CLUSTER_ROLE_LABELS variable and CLUSTER_ROLE_NAME unchanged inside that
conditional.
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml`:
- Around line 114-133: The test script hardcodes the expected replica count as
"1" causing failures when users set a different replicaCount; update the checks
to compare READY_REPLICAS and RUNNING_PODS against the chart's configured
replica count (use the Helm value, e.g. .Values.replicaCount or the chart helper
that exposes it) instead of the literal "1", referencing the variables
DEPLOYMENT_NAME, READY_REPLICAS and RUNNING_PODS in the comparisons so the test
dynamically uses the configured replica count.
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-rbac.yaml`:
- Around line 34-131: The test script unconditionally checks Role/RoleBinding
and ClusterRole/ClusterRoleBinding causing false failures; wrap the Role and
RoleBinding checks (references: ROLE_NAME, ROLEBINDING_NAME,
SERVICE_ACCOUNT_NAME) in a Helm conditional that runs only when
.Values.rbac.create is true, and wrap the ClusterRole and ClusterRoleBinding
checks (references: CLUSTER_ROLE_NAME, CLUSTER_ROLEBINDING_NAME,
SERVICE_ACCOUNT_NAME) in a Helm conditional that requires both
.Values.rbac.create and .Values.rbac.createClusterRole to be true; adjust the
jsonpath/namespace checks inside those respective conditionals so they are only
executed when the resources are actually created.
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-sa.yaml`:
- Around line 19-28: The namespaced Role's rules include incorrect core-group
entries and cluster-scoped resources: remove "deployments" and "replicasets"
from the apiGroups: [""] rule (they belong under apiGroups: ["apps"] only) and
remove "clusterroles" and "clusterrolebindings" from the namespaced Role's
resources list; then mirror the cleanup for the ClusterRole by removing any
stray "deployments"/"replicasets" entries under apiGroups: [""] so only the
apps-group rule covers those resources and cluster-scoped resources remain only
in the ClusterRole.
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-services.yaml`:
- Around line 74-110: Replace the hardcoded service names and ports with the
chart helpers and value references used elsewhere: call the same helper that
resolves service fullnames (the pattern used by GRPC_SERVICE, METRICS_SERVICE,
HEALTHZ_SERVICE) instead of the literals "argocd-agent-redis-proxy" and
"argocd-agent-resource-proxy", and read expected ports from the values
(.Values.principal.redisProxy.* and .Values.principal.resourceProxy.* or the
exact port keys used in your chart) when comparing REDIS_PORT and
RESOURCE_PROXY_PORT so the tests honor release name prefixes and configurable
ports (update the kubectl get service invocations and the port comparisons in
the redis proxy and resource proxy blocks accordingly).
---
Nitpick comments:
In
`@install/helm-repo/argocd-agent-principal/templates/principal-resource-proxy-service.yaml`:
- Around line 6-14: The service template hardcodes the service name
"argocd-agent-resource-proxy" and port 9090; change it to use the chart helper
and a values entry instead: replace the literal name with a helper call (e.g.,
use a helper similar to argocd-agent-principal.metricsServiceName — create/use
argocd-agent-principal.resourceProxyServiceName) and replace the hardcoded port
9090 with a values reference (e.g., .Values.resourceProxy.port or
.Values.service.resourceProxy.port) so operators can override it; update the
values.yaml schema with the new port key if missing and ensure templates
reference that key consistently.
In
`@install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml`:
- Around line 10-12: Replace the hardcoded image in the test ConfigMap container
named "wget" with the same configurable test image used in test-deployment.yaml;
specifically change the image field for the "wget" container to use the Helm
template values (the .Values.tests.image and .Values.tests.tag) so the test pod
honors image/tag overrides and pull policies consistently across tests.
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-labels.yaml`:
- Around line 6-7: Add the helm.sh/hook-delete-policy: before-hook-creation
annotation to the test Pod annotation blocks so Helm will remove previous test
pods; locate the annotations map in the test Pod manifests (e.g., the
annotations block in test-labels.yaml under templates/tests/) and add the
"helm.sh/hook-delete-policy": "before-hook-creation" entry alongside
"helm.sh/hook": "test" in each test Pod manifest.
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml`:
- Around line 139-155: Replace the deprecated kubectl get endpoints calls used
to populate METRICS_ENDPOINT_IP and HEALTHZ_ENDPOINT_IP with kubectl get
endpointslice queries filtered by the service name label
(kubernetes.io/service-name=<svc>) and extract the first endpoint address (use
jsonpath .items[0].endpoints[0].addresses[0].ip); update the commands that
reference METRICS_SERVICE and HEALTHZ_SERVICE in the test script so they query
EndpointSlices in the same namespace (the include
"argocd-agent-principal.namespace" template) and assign the result to
METRICS_ENDPOINT_IP and HEALTHZ_ENDPOINT_IP respectively, preserving the
existing existence checks and exit behavior.
In `@install/helm-repo/argocd-agent-principal/values.schema.json`:
- Around line 559-583: The tests.image default currently uses the legacy Bitnami
namespace; update the tests schema so the "image" property default is a
maintained image (e.g., change default from "bitnamilegacy/kubectl" to
"bitnami/kubectl" or a distroless/chainguard kubectl image) and adjust the
"description" for the "image" property to mention it uses Bitnami Secure Images
(or the chosen maintained alternative) to avoid recommending an unmaintained
image; locate the "tests" object and the "image" property in the
values.schema.json and update its "default" and "description" accordingly.
- Around line 91-119: The "resources" JSON schema is too strict: it currently
forces both "limits" and "requests" and only allows "cpu" and "memory", blocking
common Kubernetes patterns and extended resources. Relax the schema by removing
the top-level required: ["limits","requests"] and removing the required:
["cpu","memory"] inside both "limits" and "requests", and set
"additionalProperties": true for the "limits" and "requests" objects (or remove
their additionalProperties:false) so arbitrary resource keys (ephemeral-storage,
GPUs, hugepages, etc.) are allowed; keep the existing "cpu" and "memory"
property definitions but allow other properties through these changes on the
"resources", "limits", and "requests" symbols so users can supply limits-only,
requests-omitted, or extended resources.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86f5f742-b3d4-4b7a-b147-a489258aa4fb
📒 Files selected for processing (28)
install/helm-repo/argocd-agent-principal/.helmignoreinstall/helm-repo/argocd-agent-principal/Chart.yamlinstall/helm-repo/argocd-agent-principal/README.mdinstall/helm-repo/argocd-agent-principal/templates/NOTES.txtinstall/helm-repo/argocd-agent-principal/templates/_helpers.tplinstall/helm-repo/argocd-agent-principal/templates/principal-clusterrole.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-clusterrolebinding.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-deployment.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-grpc-service.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-healthz-service.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-metrics-service.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-networkpolicy.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-params-cm.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-redis-proxy-service.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-resource-proxy-service.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-role.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-rolebinding.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-sa.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-servicemonitor.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-deployment.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-labels.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-overall.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-rbac.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-sa.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-services.yamlinstall/helm-repo/argocd-agent-principal/values.schema.jsoninstall/helm-repo/argocd-agent-principal/values.yaml
Comment on lines
+354
to
+359
| - name: REDIS_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.redisSecretName }} | ||
| key: {{ .Values.redisPasswordKey }} | ||
| ports: |
Contributor
There was a problem hiding this comment.
REDIS_PASSWORD secretKeyRef is unconditional and non-optional — pod startup fails silently if the secret is absent.
Every other env var in this deployment (all ~40 ConfigMap refs) carries optional: true, yet REDIS_PASSWORD does not. If .Values.redisSecretName's Secret doesn't exist before rollout, Kubernetes will refuse to start the pod with a CreateContainerConfigError. Users who run without Redis authentication, or who haven't pre-provisioned this secret, will hit this with no clear guidance.
At minimum, gate the entry on the secret name being set:
🐛 Proposed fix
+ {{- if .Values.redisSecretName }}
- name: REDIS_PASSWORD
valueFrom:
secretKeyRef:
name: {{ .Values.redisSecretName }}
key: {{ .Values.redisPasswordKey }}
+ {{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: REDIS_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Values.redisSecretName }} | |
| key: {{ .Values.redisPasswordKey }} | |
| ports: | |
| {{- if .Values.redisSecretName }} | |
| - name: REDIS_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Values.redisSecretName }} | |
| key: {{ .Values.redisPasswordKey }} | |
| {{- end }} | |
| ports: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install/helm-repo/argocd-agent-principal/templates/principal-deployment.yaml`
around lines 354 - 359, The REDIS_PASSWORD env var is currently required
unconditionally and will cause CreateContainerConfigError if the secret is
missing; update the template so the env var is only added when a secret name is
provided and mark its secretKeyRef as optional. Specifically, wrap the
REDIS_PASSWORD env var block (the entry referencing .Values.redisSecretName and
.Values.redisPasswordKey) in a Helm conditional that checks
.Values.redisSecretName is non-empty (e.g., if .Values.redisSecretName) and/or
add optional: true under the secretKeyRef for REDIS_PASSWORD so Kubernetes will
not fail pod creation when the Secret is absent.
Contributor
Author
There was a problem hiding this comment.
Intentional — matches the existing kustomize manifest at install/kubernetes/principal/principal-deployment.yaml, where REDIS_PASSWORD also has no optional: true. The argocd-redis Secret is a standard Argo CD install artifact and a hard dependency, so failing fast on missing Secret is the right behavior. If no-auth Redis becomes a real use case, I'd gate the whole env var behind a values flag in a follow-up rather than just adding optional: true.
Contributor
There was a problem hiding this comment.
@enbiyagoral, understood — that's a reasonable design decision. Aligning with the kustomize manifest and treating the argocd-redis Secret as a hard dependency keeps the fail-fast behavior consistent. Gating the entire env var behind a values flag in a follow-up (if unauthenticated Redis ever becomes a real use case) is the right approach rather than silently tolerating a missing secret.
✏️ Learnings added
Learnt from: enbiyagoral
Repo: argoproj-labs/argocd-agent PR: 947
File: install/helm-repo/argocd-agent-principal/templates/principal-deployment.yaml:354-359
Timestamp: 2026-05-08T19:24:02.701Z
Learning: In `install/helm-repo/argocd-agent-principal/templates/principal-deployment.yaml`, the `REDIS_PASSWORD` env var intentionally omits `optional: true` and is not conditionally gated. This matches the existing kustomize manifest at `install/kubernetes/principal/principal-deployment.yaml`. The `argocd-redis` Secret is a hard dependency (standard Argo CD install artifact), and failing fast on a missing Secret is deliberate. Do not flag this as an issue in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cjcocokrisp
Repo: argoproj-labs/argocd-agent PR: 830
File: docs/operations/provenance.md:40-64
Timestamp: 2026-03-26T19:03:24.206Z
Learning: In this repo, the Quay.io image namespace is intentionally `argoprojlabs` (no hyphen), e.g. `quay.io/argoprojlabs/argocd-agent`. This differs from the GitHub org `argoproj-labs` (with hyphen). When reviewing documentation or CI/workflow files, do not treat `argoprojlabs` as a typo when it appears as part of the Quay.io registry path.
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
…ac.createClusterRole Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
…eate Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install/helm-repo/argocd-agent-principal/README.md`:
- Line 71: The README's documented default for principal.listen.host currently
shows "127.0.0.1" but should be "" to match the chart/PR objectives; update the
principal.listen.host default value in the README (the table entry for
principal.listen.host) to "" and then regenerate the chart docs using helm-docs
so the README stays in sync with the chart metadata and prevents incorrect
operator assumptions about exposure.
In
`@install/helm-repo/argocd-agent-principal/templates/tests/test-deployment.yaml`:
- Around line 5-8: Add the Helm hook delete policy to the test pods so repeated
`helm test` runs don't fail: for the manifest that defines the test pod with
name "test-deployment" and annotation "helm.sh/hook": test (and likewise in the
other two files referencing their test pod definitions), add the annotation
"helm.sh/hook-delete-policy": before-hook-creation to the annotations map so
completed test pods are removed before a new test pod with the same name is
created.
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml`:
- Around line 27-33: The test's Kubernetes label selectors (e.g.,
app.kubernetes.io/part-of=argocd-agent used when computing DEPLOYMENT_COUNT) are
too broad and can match other releases; update the selector to include
release-specific labels so it only targets this chart's resources (for example
add app.kubernetes.io/instance={{ .Release.Name }} and/or
app.kubernetes.io/name={{ include "argocd-agent-principal.name" . }} or use a
chart-provided selector helper like {{ include
"argocd-agent-principal.selectorLabels" . }}), and apply the same narrower
selector pattern to the other checks in this file (the blocks that compute
DEPLOYMENT_COUNT, SERVICES_COUNT, DAEMONSET_COUNT, POD_COUNT, and CRD_COUNT) so
each exact-count and namespace assertion only counts this release's objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 998d0fe1-38be-40dd-9439-be3e07b6ca5f
📒 Files selected for processing (13)
install/helm-repo/argocd-agent-principal/README.mdinstall/helm-repo/argocd-agent-principal/templates/_helpers.tplinstall/helm-repo/argocd-agent-principal/templates/principal-networkpolicy.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-redis-proxy-service.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-resource-proxy-service.yamlinstall/helm-repo/argocd-agent-principal/templates/principal-sa.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-deployment.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-labels.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-overall.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-rbac.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-sa.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-services.yamlinstall/helm-repo/argocd-agent-principal/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- install/helm-repo/argocd-agent-principal/values.yaml
- install/helm-repo/argocd-agent-principal/templates/_helpers.tpl
| | principal.jwt.secretName | string | `"argocd-agent-jwt"` | Name of the Secret containing the JWT signing key. | | ||
| | principal.keepAlive.minInterval | string | `"0"` | Minimum keep-alive interval for gRPC connections. | | ||
| | principal.labelSelector | string | `""` | Kubernetes label selector to restrict which resources the principal watches. | | ||
| | principal.listen.host | string | `"127.0.0.1"` | Interface address to listen on. Use "127.0.0.1" for header-based auth (prevents bypass); use "" (all interfaces) only with mTLS. | |
Contributor
There was a problem hiding this comment.
Align principal.listen.host default with chart behavior
Line 71 says the default is "127.0.0.1", but the PR objectives call out "" as the default. Please reconcile this value in the README (and regenerate via helm-docs if needed) so operators don’t deploy with incorrect exposure assumptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install/helm-repo/argocd-agent-principal/README.md` at line 71, The README's
documented default for principal.listen.host currently shows "127.0.0.1" but
should be "" to match the chart/PR objectives; update the principal.listen.host
default value in the README (the table entry for principal.listen.host) to ""
and then regenerate the chart docs using helm-docs so the README stays in sync
with the chart metadata and prevents incorrect operator assumptions about
exposure.
Contributor
Author
There was a problem hiding this comment.
README and values.yaml are correct — "127.0.0.1" is intentional and matches the kustomize manifest as the secure default. The mismatch was in the PR description; I've updated it
Contributor
There was a problem hiding this comment.
@enbiyagoral, thanks for the clarification! That makes sense — "127.0.0.1" as the default is the safer choice, ensuring the principal doesn't inadvertently expose its gRPC port on all interfaces unless the operator explicitly sets it to "". I'll note this for future reviews.
✏️ Learnings added
Learnt from: enbiyagoral
Repo: argoproj-labs/argocd-agent PR: 947
File: install/helm-repo/argocd-agent-principal/README.md:71-71
Timestamp: 2026-05-08T20:00:50.217Z
Learning: In the argocd-agent-principal Helm chart (install/helm-repo/argocd-agent-principal/), the default value for `principal.listen.host` is intentionally `"127.0.0.1"` (not `""`). This matches the kustomize manifest and is the secure default. The empty string `""` (all interfaces) is only appropriate when mTLS is used. Do not flag "127.0.0.1" as incorrect for this value.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cjcocokrisp
Repo: argoproj-labs/argocd-agent PR: 830
File: docs/operations/provenance.md:40-64
Timestamp: 2026-03-26T19:03:24.206Z
Learning: In this repo, the Quay.io image namespace is intentionally `argoprojlabs` (no hyphen), e.g. `quay.io/argoprojlabs/argocd-agent`. This differs from the GitHub org `argoproj-labs` (with hyphen). When reviewing documentation or CI/workflow files, do not treat `argoprojlabs` as a typo when it appears as part of the Quay.io registry path.
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml (1)
11-13: ⚡ Quick win
busyboxis hardcoded and unpinned — inconsistent with all other test pods.Every other test Pod in this chart uses
"{{ .Values.tests.image }}:{{ .Values.tests.tag }}"for its image, making it configurable and registry-agnostic.test-configmapalone uses a barebusyboxwith no digest or version tag. This breaks air-gapped environments wheretests.imageis overridden to a private registry mirror, and the untagged image resolves non-deterministically.🛠️ Proposed fix
- - name: wget - image: busybox + - name: busybox + image: "{{ .Values.tests.image }}:{{ .Values.tests.tag }}"The test only requires
shandcat, which both the busybox and typicalkubectl-carrying test images provide.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml` around lines 11 - 13, The test-configmap Pod uses a hardcoded unpinned image "busybox" (container name 'wget') which is inconsistent and breaks air-gapped/mirrored registries; change the container image to use the chart values (replace "busybox" with "{{ .Values.tests.image }}:{{ .Values.tests.tag }}" for the 'wget' container in templates/tests/test-configMap.yaml) so the test image is configurable and consistent with other test Pods and ensure the container still runs the existing command (sh -c ...) that only requires sh and cat.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml`:
- Around line 27-52: DEPLOYMENT_COUNT and CONFIGMAP_COUNT are missing the trim
step so comparisons can fail with GNU wc output; update the commands that set
DEPLOYMENT_COUNT and CONFIGMAP_COUNT to mirror the other counters by piping the
wc output through | tr -d ' ' (like the existing SA_COUNT etc.), so the string
equality checks [ "$DEPLOYMENT_COUNT" = "1" ] and [ "$CONFIGMAP_COUNT" = "1" ]
reliably succeed; leave SERVICE_COUNT as-is (it already uses a numeric -ge
check).
---
Nitpick comments:
In
`@install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml`:
- Around line 11-13: The test-configmap Pod uses a hardcoded unpinned image
"busybox" (container name 'wget') which is inconsistent and breaks
air-gapped/mirrored registries; change the container image to use the chart
values (replace "busybox" with "{{ .Values.tests.image }}:{{ .Values.tests.tag
}}" for the 'wget' container in templates/tests/test-configMap.yaml) so the test
image is configurable and consistent with other test Pods and ensure the
container still runs the existing command (sh -c ...) that only requires sh and
cat.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70cf7ee8-0e43-41b8-b087-9b2267d78694
📒 Files selected for processing (6)
install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-deployment.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-labels.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-overall.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-rbac.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-services.yaml
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml (1)
146-150: ⚡ Quick win
sleep 10is a brittle readiness gate — considerkubectl waitinstead.A fixed 10-second delay is both a floor (too short on a slow/stressed cluster, causing false failures) and a ceiling (unnecessarily long on a fast cluster).
kubectl waitwith a timeout polls actively and exits as soon as the condition is met.♻️ Proposed refactor
- # Wait a bit for services to get endpoints - echo "Waiting for services to get endpoints..." - sleep 10 + # Wait for services to get endpoints (up to 60s) + echo "Waiting for services to get endpoints..." + kubectl wait --for=jsonpath='{.subsets[0].addresses[0].ip}' endpoints/"$METRICS_SERVICE" \ + -n {{ include "argocd-agent-principal.namespace" . }} --timeout=60s 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml` around lines 146 - 150, Replace the brittle fixed delay "sleep 10" in the "Verify service endpoints" block with an active poll using kubectl wait: identify the service names this test targets and run kubectl wait --for=condition=ready (or for the endpoints resource) for each service with a sensible --timeout (e.g. 60s) and labels if available; in other words, remove the "sleep 10" line and use kubectl wait against the endpoints/pods for the services instead so the script exits as soon as endpoints are ready or fails after the timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml`:
- Line 5: The test pod name "test-overall" is not release-scoped and can collide
across releases; update the name field in templates/tests/test-overall.yaml to
include the release scope (e.g., prepend or append {{ .Release.Name }} or use
your chart's fullname helper like the function used elsewhere such as "fullname"
or "argocd-agent-principal.fullname") so the rendered name is unique per
release, ensuring hooks and deletePolicy operate on the correct release-specific
pod.
- Around line 113-122: The test currently masks namespace violations by piping
results through grep -v "default"; remove that second filter so
WRONG_NS_RESOURCES (and the subsequent kubectl get ... | grep -v "{{ include
"argocd-agent-principal.namespace" . }}") does not exclude resources in the
default namespace—update the pipeline used to compute WRONG_NS_RESOURCES and the
kubectl listing command referenced in the if/else block so they only exclude the
chart's target namespace (WRONG_NS_RESOURCES) and do not suppress "default" or
any namespace substring matches.
---
Nitpick comments:
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml`:
- Around line 146-150: Replace the brittle fixed delay "sleep 10" in the "Verify
service endpoints" block with an active poll using kubectl wait: identify the
service names this test targets and run kubectl wait --for=condition=ready (or
for the endpoints resource) for each service with a sensible --timeout (e.g.
60s) and labels if available; in other words, remove the "sleep 10" line and use
kubectl wait against the endpoints/pods for the services instead so the script
exits as soon as endpoints are ready or fails after the timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd907801-cccb-4f1c-97f3-dc46808870b6
📒 Files selected for processing (1)
install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml
…fety Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml (1)
13-98: ⚡ Quick winOnly 9 of 48 ConfigMap keys are validated.
The PR description notes that the chart renders 48 ConfigMap keys (matching the kustomize manifest 1:1), but this test only checks 9 of them. For a test that is specifically named and scoped to ConfigMap validation, expanding coverage to include the remaining keys (or at least the non-trivial ones like
principal.listen.host,principal.tls.*,principal.allowed-namespaces, etc.) would give stronger confidence that the ConfigMap template is rendered correctly under default values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml` around lines 13 - 98, The test only verifies 9 ConfigMap keys in test-configMap.yaml's container command; expand coverage to validate all rendered keys (or at least the remaining non-trivial ones) by adding checks for principal.listen.host, principal.listen.port (already), principal.log.level, principal.metrics.port, principal.healthz.port, principal.namespace, principal.auth, principal.tls.secret-name and principal.tls.* (e.g., principal.tls.enabled), principal.jwt.secret-name and principal.jwt.*, principal.allowed-namespaces, principal.redis.server.address and other principal.redis.* keys, etc.; implement this by replacing the repeated if-blocks with a single array of expected keys (e.g., EXPECTED_KEYS=( "principal.listen.host" "principal.tls.enabled" "principal.tls.secret-name" "principal.jwt.secret-name" "principal.allowed-namespaces" "principal.redis.server.address" ... )), then loop over EXPECTED_KEYS and for each check [ -f /etc/config/"$key" ] and cat it (or fail), or alternatively assert the total file count in /etc/config equals 48 to ensure full coverage; update the command block in test-configMap.yaml accordingly so the script programmatically verifies all keys.install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml (1)
28-28: ⚡ Quick winUse the chart's name helper in test selectors instead of hardcoding
argocd-agent-principal.The chart exposes
nameOverrideto customize theapp.kubernetes.io/namelabel, and the_helpers.tpldefines a helper to resolve this. These test queries hardcode the literal name, so ifnameOverrideis set, the selectors will fail to match the deployed resources. Using the same helper the manifests use keeps selectors aligned.♻️ Suggested pattern
# Count and verify all resources echo "Verifying resource counts..." + APP_NAME={{ include "argocd-agent-principal.name" . | quote }} + SELECTOR="app.kubernetes.io/name=$APP_NAME,app.kubernetes.io/instance={{ .Release.Name }}" # Count deployments - DEPLOYMENT_COUNT=$(kubectl get deployments -n {{ include "argocd-agent-principal.namespace" . }} -l "app.kubernetes.io/name=argocd-agent-principal,app.kubernetes.io/instance={{ .Release.Name }}" --no-headers | wc -l | tr -d ' ') + DEPLOYMENT_COUNT=$(kubectl get deployments -n {{ include "argocd-agent-principal.namespace" . }} -l "$SELECTOR" --no-headers | wc -l | tr -d ' ') ... - RUNNING_PODS=$(kubectl get pods -n {{ include "argocd-agent-principal.namespace" . }} -l "app.kubernetes.io/name=argocd-agent-principal,app.kubernetes.io/instance={{ .Release.Name }}" --no-headers | grep Running | wc -l | tr -d ' ') + RUNNING_PODS=$(kubectl get pods -n {{ include "argocd-agent-principal.namespace" . }} -l "$SELECTOR" --no-headers | grep Running | wc -l | tr -d ' ')Also applies to: 37-37, 46-46, 56-56, 69-69, 78-78, 92-92, 101-101, 137-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml` at line 28, Replace the hardcoded label value "argocd-agent-principal" in the kubectl selectors with the chart name helper so the selector honors nameOverride; specifically, update the selector string used to compute DEPLOYMENT_COUNT (and the identical selectors at the other listed locations) to use the helper include "argocd-agent-principal.name" . (or whatever name helper is defined in _helpers.tpl) and ensure the helper output is properly quoted/trimmed for the label selector syntax so the commands continue to run correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml`:
- Line 12: Replace the untagged image reference "image: busybox" in the test
ConfigMap/template with a pinned tag (for example "image: busybox:1.36.1" or
whatever approved stable tag your org uses) and optionally set an explicit
imagePullPolicy (e.g., "IfNotPresent") so tests are deterministic and work in
air-gapped environments; update the line containing "image: busybox" in the
test-configMap/template accordingly.
---
Nitpick comments:
In
`@install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yaml`:
- Around line 13-98: The test only verifies 9 ConfigMap keys in
test-configMap.yaml's container command; expand coverage to validate all
rendered keys (or at least the remaining non-trivial ones) by adding checks for
principal.listen.host, principal.listen.port (already), principal.log.level,
principal.metrics.port, principal.healthz.port, principal.namespace,
principal.auth, principal.tls.secret-name and principal.tls.* (e.g.,
principal.tls.enabled), principal.jwt.secret-name and principal.jwt.*,
principal.allowed-namespaces, principal.redis.server.address and other
principal.redis.* keys, etc.; implement this by replacing the repeated if-blocks
with a single array of expected keys (e.g., EXPECTED_KEYS=(
"principal.listen.host" "principal.tls.enabled" "principal.tls.secret-name"
"principal.jwt.secret-name" "principal.allowed-namespaces"
"principal.redis.server.address" ... )), then loop over EXPECTED_KEYS and for
each check [ -f /etc/config/"$key" ] and cat it (or fail), or alternatively
assert the total file count in /etc/config equals 48 to ensure full coverage;
update the command block in test-configMap.yaml accordingly so the script
programmatically verifies all keys.
In `@install/helm-repo/argocd-agent-principal/templates/tests/test-overall.yaml`:
- Line 28: Replace the hardcoded label value "argocd-agent-principal" in the
kubectl selectors with the chart name helper so the selector honors
nameOverride; specifically, update the selector string used to compute
DEPLOYMENT_COUNT (and the identical selectors at the other listed locations) to
use the helper include "argocd-agent-principal.name" . (or whatever name helper
is defined in _helpers.tpl) and ensure the helper output is properly
quoted/trimmed for the label selector syntax so the commands continue to run
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88957f0d-6bd7-4fd6-96b9-5d74e04b5825
📒 Files selected for processing (6)
install/helm-repo/argocd-agent-principal/templates/tests/test-configMap.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-deployment.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-labels.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-overall.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-rbac.yamlinstall/helm-repo/argocd-agent-principal/templates/tests/test-services.yaml
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
Collaborator
|
@anandrkskd Can you PTAL at this one? |
anandrkskd
reviewed
Jun 1, 2026
anandrkskd
left a comment
anandrkskd
left a comment
Contributor
There was a problem hiding this comment.
left few nitpicking comments, rest LGTM
enbiyagoral
added a commit
to enbiyagoral/argocd-agent
that referenced
this pull request
Jun 1, 2026
Signed-off-by: Enbiya Goral <100806254+enbiyagoral@users.noreply.github.com>
enbiyagoral
force-pushed
the
feat/principal-helm-chart
branch
from
7bc1a01 to
34b75b9
Compare
Contributor
Author
Thanks for the review @anandrkskd, all three fixed in 34b75b9. |
anandrkskd
approved these changes
Jun 1, 2026
anandrkskd
left a comment
anandrkskd
left a comment
Contributor
There was a problem hiding this comment.
Thanks for your contribution @enbiyagoral
LGTM
Contributor
Author
Thanks! |
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
5 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 does this PR do / why we need it:
Adds a proper Helm chart for the Principal component. The Agent already had one (
install/helm-repo/argocd-agent-agent/) but Principal was kustomize-only, which was a bit awkward for anyone running their stack through Helm/ArgoCD.This continues #512 from @ncr38. They wrote the initial scaffold and in the PR comments mentioned they'd be ok with someone taking it over to finish it, so I picked it up from there.
What's different from the initial scaffold:
values.yamlrewritten — split into clear sections (image, service, principal config, redisProxy, resourceProxy, rbac, networkPolicy, serviceMonitor, tests). Addedvalues.schema.jsonso bad input fails fast at install time instead of producing a broken render.install/kubernetes/principal/principal-deployment.yaml. Same env vars, same volume mounts, same probes, same security context. ConfigMap keys also match 1:1 (48 keys). I did a literal diff while doing this, only intentional difference left isimagePullPolicy: IfNotPresentinstead ofAlways, which I think is the more reasonable default for a chart.helm installand check the deployment is healthy, services have the right ports/selectors, RBAC bindings reference the right SA/Role, ConfigMap exists, labels are consistent, and a final overall test that counts resources end-to-end.NOTES.txtintotemplates/(it was at the chart root before so it wasn't being rendered after install). Also added.helmignore.Which issue(s) this PR fixes:
Fixes #894
How to test changes / Special notes to the reviewer:
Tested in a real cluster — chart deploys cleanly, principal comes up healthy, an agent connects to it over mTLS and stays connected through reconnects. Logs on both sides confirm cert validation, agent identity extraction from the client cert, and the event stream is established. So the actual runtime flow is verified, not just template rendering.
A couple of smaller things worth flagging:
principal.listen.hostdefaults to"127.0.0.1", same as the kustomize manifest (intentional security default to prevent header-auth bypass). During my cluster test I had to--set principal.listen.host=""so the agent could reach the principal from another pod — worth being aware that the secure default isn't the cross-pod-friendly default, and""should only be used together with mTLS.wc -lon ajsonpathresult, which always returns 0 (no trailing newline). Switched to checking the IP string is non-empty.Lighter-weight checks:
helm lint --strictis clean (only the icon-recommended info)helm templaterenders across defaults, everything-disabled, everything-enabled, tests-onimage.repository)install/kubernetes/principal/principal-params-cm.yaml(48/48)Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores