Conversation
|
@jhernand: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4124396 to
7218e3b
Compare
WalkthroughThis PR adds a draft-2020-12 JSON Schema for the Helm chart values, updates values.yaml to use empty arrays for credential lists, modifies the controller deployment template to conditionally render projected credential volume sources (emitting [] when none are provided), adds Ginkgo tests that run helm lint with a populated values file, and adds a composite setup-helm action plus workflow inserts to install Helm in CI. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/service/templates/controller/deployment.yaml (1)
16-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
requireddoes not reject an empty credential list, and theelse []branch likely produces invalid/rejected YAML.Two related problems compound here:
1.
requiredpasses for empty slices.
Helm'srequiredfunction (from sprig) only fails forniland"". An empty YAML list (controllerCredentials: []) is a non-nil, non-string value, sorequiredsucceeds and$authControllerCredentialsis set to[]. Because Go templates treat an empty slice as falsy,{{ if $authControllerCredentials }}then takes theelsebranch.2. The
elsebranch renders ambiguous / rejected YAML.
In the else branch,[]is emitted at the same 10-space indentation assources:. In a YAML block mapping, a value on a subsequent line must be more-indented than the block context for block nodes; placing a flow sequence[]at the same column is ambiguous and may parse as a sibling mapping entry (causing a parse error) rather than the value ofsources. More critically, even if accepted by the YAML parser assources: [], Kubernetes rejects a projected volume with an emptysourceslist at admission time.Replace the silent
[]fallback with an explicit failure so users get a clear error athelm lint/helm installtime rather than a cryptic Kubernetes admission error:🛡️ Proposed fix: fail explicitly for empty credential lists
- {{- $authControllerCredentials := required "auth.controllerCredentials is required" .Values.auth.controllerCredentials }} + {{- $authControllerCredentials := .Values.auth.controllerCredentials }} + {{- if not $authControllerCredentials }} + {{- fail "auth.controllerCredentials must not be empty" }} + {{- end }}And similarly for
idp.credentials:- {{- $idpCredentials := required "idp.credentials is required" .Values.idp.credentials }} + {{- $idpCredentials := .Values.idp.credentials }} + {{- if not $idpCredentials }} + {{- fail "idp.credentials must not be empty" }} + {{- end }}Then remove the
{{ else }}/[]/{{ end }}blocks (lines 69–71 and 96–98) since the template will now never reachrangewith an empty list.Also consider adding a test case with empty credentials to verify the
failpath.Also applies to: 18-18, 48-71, 75-98
🤖 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 `@charts/service/templates/controller/deployment.yaml` at line 16, The current use of required to set $authControllerCredentials allows empty lists and the template's else branch emits an invalid/ambiguous YAML `[]`; change the logic to explicitly reject empty slices: keep the existing assignment to $authControllerCredentials, then add an immediate check like `{{ if or (not $authControllerCredentials) (eq (len $authControllerCredentials) 0) }}{{ fail "auth.controllerCredentials must be provided and non-empty" }}{{ end }}` so Helm fails at render time rather than emitting `[]`; apply the same pattern for the idp.credentials variable and remove the corresponding `{{ else }}`/`[]`/`{{ end }}` branches (the ranges that assumed a non-empty list) so the template never renders an empty `sources` list.
🤖 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 `@charts/service/values.schema.json`:
- Line 2: The schema currently declares "$schema":
"https://json-schema.org/draft/2020-12/schema" and uses $defs (e.g., refs like
"#/$defs/credentialSources"), which breaks validation on Helm <3.18.0; either
update the project's Helm requirement to 3.18.0+ (so the 2020-12 schema and
$defs remain unchanged) or convert the JSON Schema to draft-07 by changing
"$schema" to "https://json-schema.org/draft-07/schema#", rename every "$defs"
object to "definitions", and update all $ref targets from "#/$defs/..." to
"#/definitions/..." (pay special attention to refs used for
auth.controllerCredentials, database.connection and idp.credentials) so the
schema validates correctly on Helm 3.8.x.
In `@internal/chart/chart_suite_test.go`:
- Around line 75-79: The test's assertion message uses the wrong variable: the
Expect(err).ToNot(HaveOccurred(), "Chart file '%s' doesn't exist", chartDir)
call reports chartDir instead of the missing file path; update that Expect
invocation to pass chartFile (the path to Chart.yaml) as the formatting argument
so the failure message shows the actual missing file, not the parent directory,
and keep the rest of the assertion intact.
---
Outside diff comments:
In `@charts/service/templates/controller/deployment.yaml`:
- Line 16: The current use of required to set $authControllerCredentials allows
empty lists and the template's else branch emits an invalid/ambiguous YAML `[]`;
change the logic to explicitly reject empty slices: keep the existing assignment
to $authControllerCredentials, then add an immediate check like `{{ if or (not
$authControllerCredentials) (eq (len $authControllerCredentials) 0) }}{{ fail
"auth.controllerCredentials must be provided and non-empty" }}{{ end }}` so Helm
fails at render time rather than emitting `[]`; apply the same pattern for the
idp.credentials variable and remove the corresponding `{{ else }}`/`[]`/`{{ end
}}` branches (the ranges that assumed a non-empty list) so the template never
renders an empty `sources` list.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 513fa44c-59ff-4dae-ab53-050ab5439146
📒 Files selected for processing (5)
charts/service/templates/controller/deployment.yamlcharts/service/values.schema.jsoncharts/service/values.yamlinternal/chart/chart_lint_test.gointernal/chart/chart_suite_test.go
8ce96d5 to
af6f7b0
Compare
Add a `values.schema.json` file to validate the service chart's `values.yaml` at lint and install time. The schema enforces types, enums, and structure for all values including the reusable ConfigMap/Secret credential source pattern used by `auth.controllerCredentials`, `database.connection`, and `idp.credentials`. Default `auth.controllerCredentials` and `idp.credentials` to empty arrays instead of null so the schema can use a plain array type rather than a `oneOf` with null. Guard the controller deployment template's `range` blocks with `if` checks so that empty credential lists render valid YAML. Fix `controllerCredentials` in the auth rules test to use an array of sources instead of a bare object, matching the credential sources schema. Add a Ginkgo test in `internal/chart` that runs `helm lint` against the service chart with all required values populated, catching schema and template regressions automatically. Add a `setup-helm` composite action that installs Helm v4.1.4 via `azure/setup-helm@v5`, and use it in the unit test and integration test jobs so they run against a pinned Helm 4 version. Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
af6f7b0 to
f372111
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/chart/chart_suite_test.go`:
- Around line 50-58: BeforeSuite currently only builds the test logger (via
logging.NewLogger() and variable logger) but does not perform the required auth
and database setup mandated by suite rules; update the BeforeSuite to also
initialize the authentication subsystem and test database (e.g., call your
package's auth initialization function and test DB setup function such as
auth.Initialize(...) or auth.InitForTests(...) and db.InitTestDB(...) /
database.SetupTestDB(...) or equivalent), capture and Expect any returned errors
like you do for the logger, and ensure teardown/closure is handled in AfterSuite
if needed so the suite fully conforms to the repo guidelines.
- Around line 82-85: The test currently uses
Expect(helmVersion).To(MatchRegexp("^v4\\."), ...) which incorrectly rejects
Helm v5+; update the assertion so it accepts v4 and any higher major version by
changing the regex passed to MatchRegexp (referencing Expect, helmVersion,
MatchRegexp) to something like "^v(?:[4-9]|[1-9][0-9]+)\\." so versions
4,5,...10+ are allowed (or alternatively parse helmVersion as a semantic version
and assert major >= 4 if you prefer a parsing approach).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e90d1baa-1671-4fc9-a05a-e7aa58d782aa
📒 Files selected for processing (8)
.github/actions/setup-helm/action.yaml.github/workflows/check-pull-request.yamlcharts/service/templates/controller/deployment.yamlcharts/service/values.schema.jsoncharts/service/values.yamlinternal/auth/auth_rules_test.gointernal/chart/chart_lint_test.gointernal/chart/chart_suite_test.go
✅ Files skipped from review due to trivial changes (1)
- .github/actions/setup-helm/action.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/chart/chart_lint_test.go
- charts/service/templates/controller/deployment.yaml
- charts/service/values.yaml
- charts/service/values.schema.json
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Review
Well-structured PR — the JSON schema is thorough, the $defs pattern for credential sources avoids repetition, and the helm lint test catches regressions automatically. Two CodeRabbit findings already addressed (error message arg, Helm 4 version gate).
| Category | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Important | 1 |
| 💡 Suggestion | 0 |
🟡 Empty credentials case not tested — else [] may render invalid YAML
File: charts/service/templates/controller/deployment.yaml:68-70 and internal/chart/chart_lint_test.go
The {{ else }} [] {{ end }} branch handles empty credential lists, but the helm lint test always provides non-empty credentials. Without whitespace trimming ({{-), the rendered output for empty credentials would be:
sources:
[]
The blank lines between sources: and [] may cause YAML parsing issues (blank lines can terminate a mapping value in some parsers). CodeRabbit flagged this too.
Recommendation: Add a second lint test case with empty credentials (controllerCredentials: [], credentials: []) to verify the template renders valid YAML in that path. If it fails, switch to {{- else }} [] {{- end }} to trim the whitespace.
Overall: LGTM pending the empty-credentials test. The schema is comprehensive, the CI integration is clean, and the credential source $defs pattern is well-designed for reuse.
3 participants
Summary
values.schema.jsonfile to validate the service chart'svalues.yamlat lint andinstall time, enforcing types, enums, and structure for all values including the reusable
ConfigMap/Secret credential source pattern.
auth.controllerCredentialsandidp.credentialsto empty arrays instead of null,and guard the controller deployment template's
rangeblocks withifchecks so emptycredential lists render valid YAML.
internal/chartthat runshelm lintagainst the service chart withall required values populated, catching schema and template regressions automatically.
Test plan
ginkgo run -v internal/chartpasses (helm lint succeeds with valid values).helm lint charts/service/with real deployment values still works.helm install --dry-runrenders correct manifests.Summary by CodeRabbit
New Features
Improvements
Tests
Chores