feat(helm): add ClusterProfile controller chart#22
Draft
kahirokunn wants to merge 1 commit into
Draft
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:
📝 WalkthroughWalkthroughAdds a Helm chart for ChangesHelm Chart Introduction
Manifest RBAC fixes and cache normalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yaml (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the extra namespace RBAC path in this hook.
This only asserts the release-namespace
Role/RoleBinding.templates/role.yamlandtemplates/rolebinding.yamlalso render additional objects for each extracontroller.clusterProfileNamespaces, so a break there would currently passhelm test. Consider iterating those namespaces here and checking each rendered binding as well.🤖 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-clusterprofile-controller/templates/tests/test-rbac.yaml` around lines 20 - 24, The RBAC test hook only verifies the release-namespace Role and RoleBinding, so it misses the extra namespace RBAC objects rendered by templates/role.yaml and templates/rolebinding.yaml for each controller.clusterProfileNamespaces entry. Update the helm test script in test-rbac.yaml to iterate over controller.clusterProfileNamespaces and assert the expected Role and RoleBinding exist in each namespace, alongside the existing checks. Use the existing fullname, namespace, and serviceAccountName template helpers to keep the checks aligned with the rendered resource names.
🤖 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 @.github/workflows/ci.yml:
- Around line 102-110: Update the Helm validation job in the CI workflow to run
the new `make helm-lint` target alongside the existing schema and helm-docs
checks. Add a step in the same job where `Validate values schema` and `Validate
helm-docs generation` are defined so chart templates and metadata are linted
before merge. Use the existing job/step structure in `.github/workflows/ci.yml`
and keep the new lint step consistent with the current `make`-based validation
flow.
- Around line 125-136: The Check chart version was bumped step in ci.yml calls
yq without ensuring it is available in that job. Update the check-chart-version
logic so the runner installs or sets up yq before BASE_VERSION and HEAD_VERSION
are read, since the validate-helm-charts setup does not apply here. Keep the fix
within the same workflow job and use the existing Check chart version was bumped
step as the insertion point.
- Around line 130-135: The version comparison shell step is interpolating
github.base_ref directly inside the script, which can be misparsed if the ref
contains shell metacharacters. Move the base ref into an env variable for this
step in the workflow, then update the git cat-file and git show commands in the
same script to reference $BASE_REF instead of the GitHub expression, keeping the
commands fully quoted and located around the chart version check logic.
In `@hack/validate-values-schema.sh`:
- Around line 61-65: The YAML path walker in validate-values-schema.sh only uses
paths(scalars), so empty objects and arrays are never validated against
values.schema.json. Update the jq helper inside the validation script to also
traverse object and array defaults, not just scalars, so paths like affinity,
podAnnotations, and tolerations are included in the generated path list. Keep
the change localized to the paths_to_strings logic and ensure the resulting path
set still emits dot-joined paths for schema checking.
In `@install/helm-repo/argocd-clusterprofile-controller/Chart.yaml`:
- Line 6: The Chart.yaml appVersion still points to the unreleased placeholder,
which makes the default image tag resolve to a non-release value. Update the
appVersion in the Helm chart so the fallback used by values.yaml and the chart’s
image.tag logic resolves to a real release tag for the controller. Keep the
change localized to the chart metadata and ensure the default install path no
longer depends on any packaging rewrite.
In `@install/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tpl`:
- Around line 200-216: The Lease RBAC in the helpers template is being emitted
unconditionally, even when controller.enableLeaderElection is disabled by
default. Update the relevant helper in _helpers.tpl so the
coordination.k8s.io/leases rules are only rendered when leader election is
enabled, using the existing controller.enableLeaderElection gate around the
lease-related permissions in the RBAC block.
In `@install/helm-repo/argocd-clusterprofile-controller/templates/role.yaml`:
- Around line 11-25: The Role template for the release namespace currently only
applies localRules and skips the watched-resource Role when the namespace
appears in controller.clusterProfileNamespaces, which leaves the controller
without clusterProfileRules in its own namespace. Update the logic in role.yaml
around the localRules and clusterProfileRules rendering so that the release
namespace also receives the watched-resource permissions, either by merging
clusterProfileRules into the first Role or by removing the release-namespace
exclusion in the range over namespaces.
In `@install/helm-repo/argocd-clusterprofile-controller/values.schema.json`:
- Around line 115-121: The schema for the controller log settings is too loose:
`logFormat` and `logLevel` in the values schema currently accept any string even
though the documented contract only allows specific values. Update the schema
definitions for `logFormat` and `logLevel` to use the matching enum constraints
so validation enforces `json|text` and `debug|info|warn|error`; keep the change
localized to the `logFormat` and `logLevel` entries in the schema.
In `@README.md`:
- Around line 148-152: The top-level Helm validation quick-start in README.md is
missing the helm lint step that the chart guide uses. Update the validation
block to include make helm-lint alongside the existing make
validate-values-schema and make generate-helm-docs steps so the shortcut matches
the workflow in install/helm-repo/docs/develop-chart.md.
---
Nitpick comments:
In
`@install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yaml`:
- Around line 20-24: The RBAC test hook only verifies the release-namespace Role
and RoleBinding, so it misses the extra namespace RBAC objects rendered by
templates/role.yaml and templates/rolebinding.yaml for each
controller.clusterProfileNamespaces entry. Update the helm test script in
test-rbac.yaml to iterate over controller.clusterProfileNamespaces and assert
the expected Role and RoleBinding exist in each namespace, alongside the
existing checks. Use the existing fullname, namespace, and serviceAccountName
template helpers to keep the checks aligned with the rendered resource names.
🪄 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: 93240902-2379-4812-85dd-618d113e6c5f
📒 Files selected for processing (30)
.github/workflows/ci.ymlMakefileREADME.mdartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yamlartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yamlartifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yamlartifacts/manifests/install.yamlhack/validate-values-schema.shinstall/helm-repo/argocd-clusterprofile-controller/.helmignoreinstall/helm-repo/argocd-clusterprofile-controller/Chart.yamlinstall/helm-repo/argocd-clusterprofile-controller/README.mdinstall/helm-repo/argocd-clusterprofile-controller/README.md.gotmplinstall/helm-repo/argocd-clusterprofile-controller/crds/clusterprofiles.multicluster.x-k8s.io.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tplinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrole.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/deployment.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/networkpolicy.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/role.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/rolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/service.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/serviceaccount.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-sa.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-workload.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/validations.yamlinstall/helm-repo/argocd-clusterprofile-controller/values.schema.jsoninstall/helm-repo/argocd-clusterprofile-controller/values.yamlinstall/helm-repo/docs/develop-chart.mdmain.go
💤 Files with no reviewable changes (2)
- artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yaml
- artifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yaml
kahirokunn
force-pushed
the
split/helm-controller-chart
branch
from
ef92b1e to
14f88bd
Compare
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-clusterprofile-controller/templates/deployment.yaml`:
- Around line 65-69: The Deployment template exposes the health port but never
uses it for pod probing, so the controller can appear healthy even when wedged.
Update the pod spec in the deployment template to add both livenessProbe and
readinessProbe on the container, and wire them to the existing health
endpoint/healthz port exposed by argocd-clusterprofile-controller.probePort so
Kubernetes can detect and restart unhealthy pods.
In
`@install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yaml`:
- Line 13: The test ServiceAccount used by the RBAC test only has namespaced
permissions, so the `watchesAllClusterProfiles` path cannot read cluster-scoped
RBAC resources. Update the test setup in the `test-rbac`/`test-sa` templates to
grant cluster-scoped read access by adding a dedicated `ClusterRole` and
`ClusterRoleBinding` for the test ServiceAccount, or switch the test pod to a
ServiceAccount that already has those cluster-wide read permissions.
In
`@install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-workload.yaml`:
- Around line 20-25: The Helm test in the workload test script only checks that
the Deployment object exists, so update the test logic in the test-workload
template to verify rollout readiness instead. In the test command block that
uses DEPLOYMENT and NAMESPACE, replace the plain kubectl get deployment check
with kubectl rollout status on the controller Deployment, using a timeout so the
test fails if the rollout never becomes Available, then keep the optional
Service check under the existing metrics-enabled condition.
🪄 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: b33686ff-e13c-45b7-8e2f-f581c232b302
📒 Files selected for processing (29)
.github/workflows/ci.ymlMakefileREADME.mdartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yamlartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yamlartifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yamlartifacts/manifests/install.yamlhack/validate-values-schema.shinstall/helm-repo/argocd-clusterprofile-controller/.helmignoreinstall/helm-repo/argocd-clusterprofile-controller/Chart.yamlinstall/helm-repo/argocd-clusterprofile-controller/README.mdinstall/helm-repo/argocd-clusterprofile-controller/README.md.gotmplinstall/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tplinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrole.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/deployment.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/networkpolicy.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/role.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/rolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/service.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/serviceaccount.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-sa.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-workload.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/validations.yamlinstall/helm-repo/argocd-clusterprofile-controller/values.schema.jsoninstall/helm-repo/argocd-clusterprofile-controller/values.yamlinstall/helm-repo/docs/develop-chart.mdmain.go
💤 Files with no reviewable changes (2)
- artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yaml
- artifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yaml
✅ Files skipped from review due to trivial changes (5)
- install/helm-repo/argocd-clusterprofile-controller/README.md
- install/helm-repo/argocd-clusterprofile-controller/.helmignore
- install/helm-repo/docs/develop-chart.md
- README.md
- install/helm-repo/argocd-clusterprofile-controller/README.md.gotmpl
🚧 Files skipped from review as they are similar to previous changes (8)
- install/helm-repo/argocd-clusterprofile-controller/values.yaml
- artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yaml
- artifacts/manifests/install.yaml
- .github/workflows/ci.yml
- main.go
- install/helm-repo/argocd-clusterprofile-controller/values.schema.json
- hack/validate-values-schema.sh
- install/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tpl
kahirokunn
force-pushed
the
split/helm-controller-chart
branch
3 times, most recently
from
476d46a to
c45dc2e
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
kahirokunn
force-pushed
the
split/helm-controller-chart
branch
from
c45dc2e to
f15a7f7
Compare
There was a problem hiding this comment.
Pull request overview
Adds a first-party Helm chart for deploying the ClusterProfile controller (addressing issue #14), alongside CI and tooling updates to lint/validate the chart and align generated manifests.
Changes:
- Introduces a new Helm chart (
install/helm-repo/argocd-clusterprofile-controller) with templates for deployment, RBAC, networking, and optional Helm tests, plus generated chart docs. - Adds Makefile targets and a validation script to lint charts and verify
values.yaml↔values.schema.jsoncoverage. - Updates CI to conditionally run Go vs. Helm validation jobs based on changed paths, and adjusts controller cache namespace handling.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents Helm installation and chart validation targets; clarifies CRD ownership when using Helm. |
| Makefile | Adds Helm lint/docs/schema validation targets and chart directory discovery. |
| main.go | Refactors namespace normalization/cache option logic for watched namespaces. |
| install/helm-repo/docs/develop-chart.md | Adds contributor documentation for chart versioning and local validation. |
| install/helm-repo/argocd-clusterprofile-controller/values.yaml | Adds chart default values (image, controller args, RBAC, service, network policy, tests). |
| install/helm-repo/argocd-clusterprofile-controller/values.schema.json | Adds JSON schema for chart values validation. |
| install/helm-repo/argocd-clusterprofile-controller/templates/validations.yaml | Adds template hook to fail fast on invalid values. |
| install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-workload.yaml | Adds Helm test pod that checks deployment rollout and optional metrics service. |
| install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-sa.yaml | Adds Helm test service account and namespaced RBAC for test pods. |
| install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yaml | Adds Helm test that validates RBAC resources depending on watch scope. |
| install/helm-repo/argocd-clusterprofile-controller/templates/serviceaccount.yaml | Adds service account template controlled by values. |
| install/helm-repo/argocd-clusterprofile-controller/templates/service.yaml | Adds optional metrics Service template. |
| install/helm-repo/argocd-clusterprofile-controller/templates/rolebinding.yaml | Adds RoleBinding templates across watched namespaces. |
| install/helm-repo/argocd-clusterprofile-controller/templates/role.yaml | Adds Role templates for release and watched namespaces. |
| install/helm-repo/argocd-clusterprofile-controller/templates/networkpolicy.yaml | Adds optional NetworkPolicy for metrics ingress. |
| install/helm-repo/argocd-clusterprofile-controller/templates/deployment.yaml | Adds controller Deployment template including cmd-params wiring and security context defaults. |
| install/helm-repo/argocd-clusterprofile-controller/templates/clusterrolebinding.yaml | Adds conditional ClusterRoleBinding for cluster-wide ClusterProfile watches. |
| install/helm-repo/argocd-clusterprofile-controller/templates/clusterrole.yaml | Adds conditional ClusterRole for cluster-wide ClusterProfile permissions. |
| install/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tpl | Adds naming helpers, ports helpers, RBAC rule helpers, and values validation logic. |
| install/helm-repo/argocd-clusterprofile-controller/README.md.gotmpl | Adds helm-docs template to generate chart README. |
| install/helm-repo/argocd-clusterprofile-controller/README.md | Adds generated chart documentation (values table, metadata). |
| install/helm-repo/argocd-clusterprofile-controller/Chart.yaml | Adds chart metadata (version/appVersion, kubeVersion, maintainers). |
| install/helm-repo/argocd-clusterprofile-controller/.helmignore | Adds packaging ignore rules (including README.md.gotmpl). |
| hack/validate-values-schema.sh | Adds script to ensure values.yaml fields exist in values.schema.json. |
| artifacts/manifests/install.yaml | Updates generated install manifest RBAC and deployment container args/env layout. |
| artifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yaml | Updates generated ClusterRole permissions for clusterprofiles. |
| artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yaml | Updates generated Role permissions for clusterprofiles. |
| artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yaml | Updates generated deployment container args/image stanza. |
| .github/workflows/ci.yml | Adds change-detection job and Helm validation/version-bump checks; gates Go jobs based on detected changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kahirokunn
force-pushed
the
split/helm-controller-chart
branch
from
f15a7f7 to
5b1071b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yaml (1)
53-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert binding contents, not just resource existence.
These
kubectl get rolebinding/clusterrolebindingchecks only prove the objects were rendered. The hook still passes if a binding points at the wrong ServiceAccount or the wrongroleRef, which is the main RBAC regression this test should catch. Please add ajsonpathassertion for the expectedsubjects[*].name/subjects[*].namespaceandroleRef.name.Suggested direction
- kubectl get rolebinding {{ include "argocd-clusterprofile-controller.fullname" . | quote }} -n {{ include "argocd-clusterprofile-controller.namespace" . | quote }} + kubectl get rolebinding {{ include "argocd-clusterprofile-controller.fullname" . | quote }} \ + -n {{ include "argocd-clusterprofile-controller.namespace" . | quote }} \ + -o jsonpath='{.roleRef.kind}{"|"}{.roleRef.name}{"|"}{range .subjects[*]}{.kind}:{.name}:{.namespace}{"\n"}{end}' \ + | grep -Fx 'Role|{{ include "argocd-clusterprofile-controller.fullname" . }}|ServiceAccount:{{ include "argocd-clusterprofile-controller.serviceAccountName" . }}:{{ include "argocd-clusterprofile-controller.namespace" . }}'🤖 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-clusterprofile-controller/templates/tests/test-rbac.yaml` around lines 53 - 64, The RBAC hook in test-rbac.yaml only checks that RoleBinding and ClusterRoleBinding objects exist, so it can miss incorrect subjects or roleRef targets. Update the test commands in the RBAC test template to assert the binding contents using kubectl jsonpath, specifically validating the expected subjects[*].name and subjects[*].namespace along with roleRef.name. Keep the checks aligned with the existing binding resources created by argocd-clusterprofile-controller.fullname and clusterProfileClusterRoleName so the test fails when the ServiceAccount or role reference is wrong.
🤖 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-clusterprofile-controller/templates/role.yaml`:
- Around line 2-3: Normalize the namespace list in the RBAC templates before
rendering: the current $namespaces assignment in the role/RoleBinding templates
still uses the raw controller.clusterProfileNamespaces values, so blank or
padded entries can produce invalid namespace names or break the
release-namespace match. Update the shared namespace-building logic to trim
entries and drop empties before uniq, then reuse that normalized list
consistently in both the Role and matching RoleBinding templates.
---
Nitpick comments:
In
`@install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yaml`:
- Around line 53-64: The RBAC hook in test-rbac.yaml only checks that
RoleBinding and ClusterRoleBinding objects exist, so it can miss incorrect
subjects or roleRef targets. Update the test commands in the RBAC test template
to assert the binding contents using kubectl jsonpath, specifically validating
the expected subjects[*].name and subjects[*].namespace along with roleRef.name.
Keep the checks aligned with the existing binding resources created by
argocd-clusterprofile-controller.fullname and clusterProfileClusterRoleName so
the test fails when the ServiceAccount or role reference is wrong.
🪄 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: 2cf41833-ce5a-4577-9587-0a0e41735bfe
📒 Files selected for processing (29)
.github/workflows/ci.ymlMakefileREADME.mdartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yamlartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yamlartifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yamlartifacts/manifests/install.yamlhack/validate-values-schema.shinstall/helm-repo/argocd-clusterprofile-controller/.helmignoreinstall/helm-repo/argocd-clusterprofile-controller/Chart.yamlinstall/helm-repo/argocd-clusterprofile-controller/README.mdinstall/helm-repo/argocd-clusterprofile-controller/README.md.gotmplinstall/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tplinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrole.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/deployment.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/networkpolicy.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/role.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/rolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/service.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/serviceaccount.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-sa.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-workload.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/validations.yamlinstall/helm-repo/argocd-clusterprofile-controller/values.schema.jsoninstall/helm-repo/argocd-clusterprofile-controller/values.yamlinstall/helm-repo/docs/develop-chart.mdmain.go
💤 Files with no reviewable changes (2)
- artifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yaml
- artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yaml
✅ Files skipped from review due to trivial changes (5)
- install/helm-repo/argocd-clusterprofile-controller/.helmignore
- install/helm-repo/argocd-clusterprofile-controller/README.md.gotmpl
- install/helm-repo/docs/develop-chart.md
- install/helm-repo/argocd-clusterprofile-controller/Chart.yaml
- README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- install/helm-repo/argocd-clusterprofile-controller/README.md
- artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yaml
- artifacts/manifests/install.yaml
- install/helm-repo/argocd-clusterprofile-controller/values.yaml
- install/helm-repo/argocd-clusterprofile-controller/values.schema.json
- main.go
- .github/workflows/ci.yml
- install/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tpl
- hack/validate-values-schema.sh
kahirokunn
force-pushed
the
split/helm-controller-chart
branch
from
5b1071b to
6eca4cf
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@hack/validate-values-schema.sh`:
- Around line 79-94: The schema walker in extract_schema_paths is incorrectly
recursing into non-property schema keywords because of the fallback that treats
any object as a property map. Update the jq logic in extract_properties so it
only descends through .properties and does not use the generic else . end path,
ensuring keys like replicaCount.type and replicaCount.minimum are not included
in the valid values path set.
In `@install/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tpl`:
- Around line 107-115: The cluster profile namespace validation in the
"argocd-clusterprofile-controller.clusterProfileNamespaces" helper is happening
after trimming and filtering, which lets empty or whitespace-only raw entries
bypass "validateValues". Update the validation flow in the helper and the
related validation logic so the raw values from
.Values.controller.clusterProfileNamespaces are checked first, then normalized
only after invalid entries are rejected, keeping the Deployment’s
--cluster-profile-namespaces input aligned with the validation rules.
In
`@install/helm-repo/argocd-clusterprofile-controller/templates/deployment.yaml`:
- Around line 39-40: The controller probe listener currently accepts
loopback-only values via controller.probeAddr, which can make the pod fail
readiness/liveness because the probes connect over the pod IP. Update the Helm
chart in the deployment template to validate or reject localhost/127.0.0.1-style
probeAddr values, or override them so the probe server binds to all interfaces,
and ensure the probe configuration used by the readiness/liveness probes stays
compatible with the controller startup flags.
In
`@install/helm-repo/argocd-clusterprofile-controller/templates/rolebinding.yaml`:
- Around line 4-18: The RoleBinding template currently always creates a binding
in the release namespace, which can grant RBAC outside the configured watch
scope when `controller.clusterProfileNamespaces` does not include that
namespace. Update `rolebinding.yaml` to conditionally render the RoleBinding
using `watchesReleaseNamespace` so the binding is only created when the
controller is supposed to watch the release namespace, and keep the existing
`RoleBinding`/`serviceAccountName` references intact while gating the whole
resource.
In
`@install/helm-repo/argocd-clusterprofile-controller/templates/tests/test-sa.yaml`:
- Around line 19-28: The test ServiceAccount RBAC is missing read access to
ServiceAccounts, which causes the Helm test to fail before its assertions run.
Update the Role in test-sa.yaml so the existing core API rule used by the test
RBAC helper also includes serviceaccounts alongside pods, services, and
endpoints. Keep the change aligned with the test setup referenced by
templates/tests/test-rbac.yaml and the SA-related access checks performed by
kubectl get serviceaccount in the test flow.
🪄 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: 19d8e1c4-5216-496e-a152-962150591e4a
📒 Files selected for processing (29)
.github/workflows/ci.ymlMakefileREADME.mdartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yamlartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yamlartifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yamlartifacts/manifests/install.yamlhack/validate-values-schema.shinstall/helm-repo/argocd-clusterprofile-controller/.helmignoreinstall/helm-repo/argocd-clusterprofile-controller/Chart.yamlinstall/helm-repo/argocd-clusterprofile-controller/README.mdinstall/helm-repo/argocd-clusterprofile-controller/README.md.gotmplinstall/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tplinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrole.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/deployment.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/networkpolicy.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/role.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/rolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/service.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/serviceaccount.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-sa.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-workload.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/validations.yamlinstall/helm-repo/argocd-clusterprofile-controller/values.schema.jsoninstall/helm-repo/argocd-clusterprofile-controller/values.yamlinstall/helm-repo/docs/develop-chart.mdmain.go
💤 Files with no reviewable changes (2)
- artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yaml
- artifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yaml
✅ Files skipped from review due to trivial changes (5)
- install/helm-repo/argocd-clusterprofile-controller/.helmignore
- install/helm-repo/docs/develop-chart.md
- install/helm-repo/argocd-clusterprofile-controller/README.md.gotmpl
- install/helm-repo/argocd-clusterprofile-controller/README.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- install/helm-repo/argocd-clusterprofile-controller/Chart.yaml
- install/helm-repo/argocd-clusterprofile-controller/values.yaml
- artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yaml
- main.go
- artifacts/manifests/install.yaml
- .github/workflows/ci.yml
- install/helm-repo/argocd-clusterprofile-controller/values.schema.json
kahirokunn
force-pushed
the
split/helm-controller-chart
branch
from
6eca4cf to
672de65
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
94-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant slash in script path.
current_diralready ends with a trailing slash (from$(dir ...)), so$(current_dir)/hack/...expands to…//hack/validate-values-schema.sh. It works on Unix but is misleading; drop the explicit/.♻️ Proposed tweak
- @$(current_dir)/hack/validate-values-schema.sh + @$(current_dir)hack/validate-values-schema.sh🤖 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 `@Makefile` at line 94, The Makefile target invoking validate-values-schema has a redundant path separator because current_dir already includes a trailing slash, so update the script reference in the validate-values-schema rule to use current_dir directly without adding another slash; this change should be made where the hack/validate-values-schema.sh command is constructed.
🤖 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.
Nitpick comments:
In `@Makefile`:
- Line 94: The Makefile target invoking validate-values-schema has a redundant
path separator because current_dir already includes a trailing slash, so update
the script reference in the validate-values-schema rule to use current_dir
directly without adding another slash; this change should be made where the
hack/validate-values-schema.sh command is constructed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be58927c-c893-4f01-a9f8-47ccc6408f23
📒 Files selected for processing (29)
.github/workflows/ci.ymlMakefileREADME.mdartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yamlartifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yamlartifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yamlartifacts/manifests/install.yamlhack/validate-values-schema.shinstall/helm-repo/argocd-clusterprofile-controller/.helmignoreinstall/helm-repo/argocd-clusterprofile-controller/Chart.yamlinstall/helm-repo/argocd-clusterprofile-controller/README.mdinstall/helm-repo/argocd-clusterprofile-controller/README.md.gotmplinstall/helm-repo/argocd-clusterprofile-controller/templates/_helpers.tplinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrole.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/clusterrolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/deployment.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/networkpolicy.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/role.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/rolebinding.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/service.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/serviceaccount.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-rbac.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-sa.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/tests/test-workload.yamlinstall/helm-repo/argocd-clusterprofile-controller/templates/validations.yamlinstall/helm-repo/argocd-clusterprofile-controller/values.schema.jsoninstall/helm-repo/argocd-clusterprofile-controller/values.yamlinstall/helm-repo/docs/develop-chart.mdmain.go
💤 Files with no reviewable changes (2)
- artifacts/manifests/cluster-rbac/clusterprofile-controller/argocd-clusterprofile-controller-clusterrole.yaml
- artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-role.yaml
✅ Files skipped from review due to trivial changes (4)
- install/helm-repo/argocd-clusterprofile-controller/README.md
- install/helm-repo/argocd-clusterprofile-controller/.helmignore
- install/helm-repo/docs/develop-chart.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- install/helm-repo/argocd-clusterprofile-controller/Chart.yaml
- artifacts/manifests/base/clusterprofile-controller/argocd-clusterprofile-controller-deployment.yaml
- install/helm-repo/argocd-clusterprofile-controller/README.md.gotmpl
- install/helm-repo/argocd-clusterprofile-controller/values.yaml
- .github/workflows/ci.yml
- hack/validate-values-schema.sh
- install/helm-repo/argocd-clusterprofile-controller/values.schema.json
- main.go
kahirokunn
force-pushed
the
split/helm-controller-chart
branch
from
f103552 to
3bec251
Compare
kahirokunn
force-pushed
the
split/helm-controller-chart
branch
13 times, most recently
from
1fb398d to
4a6be0f
Compare
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
kahirokunn
force-pushed
the
split/helm-controller-chart
branch
from
4a6be0f to
5e6ce49
Compare
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
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.
fixes: #14
Summary by CodeRabbit
"*".