[test] Add test cases for types and combinations in values.yaml #11
Conversation
WalkthroughThis change introduces a comprehensive suite of new test assets for a values and schema validation system. It adds structured test directories for basic types, complex types, lists, objects, and templates, each containing a Makefile, YAML test data, JSON schema, and documentation. Additionally, a shared Makefile ( Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Makefile (test.mk)
participant cozyvalues-gen
participant Git
Tester->>Makefile (test.mk): make test
Makefile (test.mk)->>cozyvalues-gen: generate files from values.yaml, values.schema.json, README.md
Makefile (test.mk)->>Git: git diff --quiet
alt No changes
Makefile (test.mk)-->>Tester: exit 0
else Changes detected
Makefile (test.mk)-->>Tester: show git diff, exit 1
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @NickVolynkin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a comprehensive suite of new test cases for values.yaml files, covering various data types, complex structures, and nesting. The primary goal is to ensure the cozyvalues-gen tool correctly processes and validates different configurations, thereby improving the robustness and reliability of schema generation and validation for values.yaml files.
Highlights
- Expanded Test Coverage for Basic Data Types: New test cases for
int,bool, andstringtypes, including scenarios with nullable fields and default values, are added undertest/basic-types/. - Introduction of Complex Type Testing: Dedicated tests for
quantitytypes (e.g., CPU and memory measurements) are included intest/complex-types/, validating various formats and default assignments. - Structured Testing for Nested Objects: A new test suite in
test/values-inside-object/specifically addresses the validation of nested configurations, demonstrated with a Grafana resource setup. - Foundational Test Scaffolding: Basic test structures for
listsandtemplatedirectories are established, laying groundwork for future test expansions. - Automated Test Execution: A new
test.mkfile providesgenerateandtesttargets, streamlining the process of generating schemas and validating changes usingcozyvalues-genandgit diff.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of test cases for the cozyvalues-gen tool, covering basic types, complex types, lists, templates, and nested objects. This is a great addition to ensure the tool's stability and correctness.
My review focuses on improving the quality and correctness of these test cases. I've found several areas for improvement:
- The generated JSON schemas for nullable types are incorrect across multiple test cases. They don't allow
nullvalues, which contradicts thevalues.yamlannotations. This seems to point to a bug in the generator itself. - Some test cases have incomplete schemas or documentation (
README.md). - There are minor formatting issues in some YAML files and a consistent lack of a final newline in most new text files.
I've provided specific suggestions to address these points. Fixing the schema generation for nullable types should be a priority.
| "quantityNullable": { | ||
| "description": "A nullable quantity value.", | ||
| "type": "string", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| }, |
There was a problem hiding this comment.
The schema for quantityNullable should allow null values, as indicated by the *quantity type in values.yaml. The current schema only allows a string, which would cause validation to fail for null values. This issue applies to all nullable types in this file.
"quantityNullable": {
"description": "A nullable quantity value.",
"type": ["string", "null"],
"pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",
"x-kubernetes-int-or-string": true
},
There was a problem hiding this comment.
@kvaps is this how nullable types are declared for K8s API?
| "testBoolNullable": { | ||
| "description": "Boolean variable, nullable", | ||
| "type": "boolean" | ||
| }, |
There was a problem hiding this comment.
The schema for testBoolNullable should allow null values, as indicated by the *bool type in values.yaml. The current schema only allows a boolean, which would cause validation to fail for null values. This issue applies to all nullable types in this file. Since these test files are intended to lock in the behavior of cozyvalues-gen, it seems the tool has a bug in generating schemas for nullable types.
"testBoolNullable": {
"description": "Boolean variable, nullable",
"type": ["boolean", "null"]
},
| "cpu": { | ||
| "description": "CPU resources", | ||
| "type": "string", | ||
| "default": "100m", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| }, |
There was a problem hiding this comment.
The schema for cpu resources should allow null values, as indicated by the *quantity type in values.yaml. The current schema only allows a string, which would cause validation to fail for null values. This issue applies to all nullable resource types in this file.
"cpu": {
"description": "CPU resources",
"type": ["string", "null"],
"default": "100m",
"pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",
"x-kubernetes-int-or-string": true
},
test/values-inside-object/README.md
Outdated
| ## Case Description | ||
|
|
||
|
|
There was a problem hiding this comment.
The "Case Description" section is empty. Please add a brief description of what this test case covers to improve clarity.
| ## Case Description | |
| ## Case Description | |
| Tests for values defined inside nested objects. | |
| { | ||
| "title": "Chart Values", | ||
| "type": "object", | ||
| "properties": { | ||
| "grafana": { | ||
| "description": "Configuration for Grafana", | ||
| "type": "object", | ||
| "required": [ | ||
| "db" | ||
| ], | ||
| "properties": { | ||
| "db": { | ||
| "type": "object", | ||
| "required": [ | ||
| "size" | ||
| ], | ||
| "properties": { | ||
| "size": { | ||
| "description": "Persistent Volume size for grafana database", | ||
| "type": "string", | ||
| "default": "10Gi" | ||
| } | ||
| } | ||
| }, | ||
| "resources": { | ||
| "description": "Resources configuration for grafana", | ||
| "type": "object", | ||
| "properties": { | ||
| "limits": { | ||
| "description": "Resources limits for grafana", | ||
| "type": "object", | ||
| "properties": { | ||
| "cpu": { | ||
| "description": "CPU resources", | ||
| "type": "string", | ||
| "default": "100m", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| }, | ||
| "memory": { | ||
| "description": "Memory resources", | ||
| "type": "string", | ||
| "default": "256Mi", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| } | ||
| } | ||
| }, | ||
| "requests": { | ||
| "description": "Resources requests for grafana", | ||
| "type": "object", | ||
| "properties": { | ||
| "cpu": { | ||
| "description": "CPU resources", | ||
| "type": "string", | ||
| "default": "100m", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| }, | ||
| "memory": { | ||
| "description": "Memory resources", | ||
| "type": "string", | ||
| "default": "256Mi", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
It's a common convention and POSIX standard for text files to end with a newline character. This file is missing it.
{
"title": "Chart Values",
"type": "object",
"properties": {
"grafana": {
"description": "Configuration for Grafana",
"type": "object",
"required": [
"db"
],
"properties": {
"db": {
"type": "object",
"required": [
"size"
],
"properties": {
"size": {
"description": "Persistent Volume size for grafana database",
"type": "string",
"default": "10Gi"
}
}
},
"resources": {
"description": "Resources configuration for grafana",
"type": "object",
"properties": {
"limits": {
"description": "Resources limits for grafana",
"type": "object",
"properties": {
"cpu": {
"description": "CPU resources",
"type": "string",
"default": "100m",
"pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",
"x-kubernetes-int-or-string": true
},
"memory": {
"description": "Memory resources",
"type": "string",
"default": "256Mi",
"pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",
"x-kubernetes-int-or-string": true
}
}
},
"requests": {
"description": "Resources requests for grafana",
"type": "object",
"properties": {
"cpu": {
"description": "CPU resources",
"type": "string",
"default": "100m",
"pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",
"x-kubernetes-int-or-string": true
},
"memory": {
"description": "Memory resources",
"type": "string",
"default": "256Mi",
"pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",
"x-kubernetes-int-or-string": true
}
}
}
}
}
}
}
}
}
| ## @section Grafana configuration | ||
|
|
||
| ## @field resources.cpu {*quantity} CPU resources | ||
| ## @field resources.memory {*quantity} Memory resources | ||
|
|
||
|
|
||
| ## @param grafana {grafana} Configuration for Grafana | ||
| ## @field grafana.db {grafanaDB} | ||
| ## @field grafana.db.size {string} Persistent Volume size for grafana database | ||
| ## @field grafana.resources {*grafanaResources} Resources configuration for grafana | ||
| ## @field grafanaResources.limits {*resources} Resources limits for grafana | ||
| ## @field grafanaResources.requests {*resources} Resources requests for grafana | ||
| grafana: | ||
| db: | ||
| size: 10Gi | ||
| resources: | ||
| limits: | ||
| cpu: "1" | ||
| memory: 1Gi | ||
| requests: | ||
| cpu: 100m | ||
| memory: 256Mi No newline at end of file |
There was a problem hiding this comment.
It's a common convention and POSIX standard for text files to end with a newline character. This file is missing it.
## @section Grafana configuration
## @field resources.cpu {*quantity} CPU resources
## @field resources.memory {*quantity} Memory resources
## @param grafana {grafana} Configuration for Grafana
## @field grafana.db {grafanaDB}
## @field grafana.db.size {string} Persistent Volume size for grafana database
## @field grafana.resources {*grafanaResources} Resources configuration for grafana
## @field grafanaResources.limits {*resources} Resources limits for grafana
## @field grafanaResources.requests {*resources} Resources requests for grafana
grafana:
db:
size: 10Gi
resources:
limits:
cpu: "1"
memory: 1Gi
requests:
cpu: 100m
memory: 256Mi
![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
♻️ Duplicate comments (4)
test/complex-types/Makefile (1)
1-1: Will inherit PHONY fix from parenttest.mkNo changes required here once
.PHONYand default target are added to the sharedtest.mk.test/template/Makefile (1)
1-1: Same observation astest/complex-types/MakefileThis file relies entirely on
../test.mk; the earlier fix covers it.test/lists/Makefile (1)
1-1: Will inherit PHONY fix from parenttest.mkNo further changes needed after central Makefile update.
test/template/values.yaml (1)
1-4: Same newline / explicit-null issue as intest/lists/values.yamlReplicate the fix to keep the linter green and the default value unambiguous.
test: - + null +
🧹 Nitpick comments (13)
test/values-inside-object/values.schema.json (1)
25-70: Consider tightening validation & reduce duplication
- All nested objects (
grafana,db,resources,limits,requests) currently allow arbitrary extra keys. Adding"additionalProperties": falseat each object level would surface typos early and keeps the test focused on the declared contract.
limitsandrequestsshare identical structures. Define them once via$defs/$refto cut noise, e.g.+ "$defs": { + "resourcesSpec": { + "type": "object", + "properties": { ...cpu/memory… } + } + }, ... - "limits": { ... }, - "requests": { ... } + "limits": { "$ref": "#/$defs/resourcesSpec" }, + "requests": { "$ref": "#/$defs/resourcesSpec" }This shortens future tests and shows good-schema hygiene.
test/values-inside-object/README.md (1)
1-4: Fill the empty “Case Description” sectionThe header is present but no narrative is provided. Two-sentence context (what is being validated and why) helps future readers scan the test catalogue.
test/values-inside-object/values.yaml (2)
18-20: Quote all resource quantities for consistency
memory: 1Giis unquoted while its neighbours are strings. Small but visible inconsistency; quote it or drop quotes everywhere.- memory: 1Gi + memory: "1Gi"
22-22: Add missing newline at EOFYAML-lint flagged the file. Add a trailing newline to keep linters green.
test/basic-types/Makefile (1)
1-1: Declare phony targets to silencecheckmakenoise
checkmakewarnings stem from missing.PHONYdeclarations for targets provided via../test.mk.
Add a minimal stub to keep the linter quiet without duplicating rules:include ../test.mk + +.PHONY: all test cleantest/lists/values.yaml (1)
1-4: Terminate file with newline & makenullexplicitYAMLlint flags the missing trailing newline.
Also, an explicitnullbetter conveys intent than an empty scalar.test: - + # explicitly null by default + null +test/template/values.schema.json (1)
1-9: Missing$schemaand unspecified type fortest
- Add the
$schemakey (see earlier comment).- If the property is meant to be an unconstrained object, add
"type": ["object", "null"]; if you want full flexibility, leave it but document the reason to avoid confusion.{ + "$schema": "https://json-schema.org/draft-07/schema#", "title": "Chart Values", "type": "object", "properties": { "test": { "description": "Test variable" + /* optionally add "type": ["object", "null"] */ } } }test/lists/README.md (1)
1-4: Populate the “Case Description” section or remove the stubThe header is present but the section is empty. Either add a concise description of the list–specific test scenario or drop the header to avoid dead-weight documentation.
test/template/README.md (1)
1-4: Provide an actual case descriptionSame comment as for the
test/listsREADME: the heading is present but no information is supplied. Briefly state what this template test is intended to verify.test/complex-types/values.yaml (1)
40-42: Trim the extra blank line at EOFYAMLlint warns about the superfluous empty line at the end of the file.
-quantityNullableDefaultRam: "500MiB" - +quantityNullableDefaultRam: "500MiB"test/basic-types/README.md (1)
37-41: Minor formatting: back-ticks for empty stringFor consistency with other default values, wrap the empty string in back-ticks:
| `testStringEmpty` | … | `string` | `` → `""` |test/basic-types/values.yaml (1)
7-10: Prefer explicitnullfor nullable parametersBare keys (
testIntNullable:/testIntDefaultNullable:) parse asnull, but Helm-/YAML linters sometimes flag these as “missing value”.
Being explicit avoids false positives and improves readability.-testIntNullable: -testIntDefaultNullable: 10 +testIntNullable: null +testIntDefaultNullable: 10 # keep defaulttest/complex-types/values.schema.json (1)
5-66: DRY: factor the repeated quantity pattern into$defsThe identical regex is copied 14 ×. Defining it once improves maintainability:
{ "title": "Chart Values", "type": "object", + "$defs": { + "quantityPattern": { + "type": "string", + "pattern": "^(\\\\+|-)?(([0-9]+(\\\\.[0-9]*)?)|(\\\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\\\+|-)?(([0-9]+(\\\\.[0-9]*)?)|(\\\\.[0-9]+))))?$", + "x-kubernetes-int-or-string": true + } + }, "properties": { - "quantityDefaultCpuShare": { - "type": "string", - ... - }, + "quantityDefaultCpuShare": { "$ref": "#/$defs/quantityPattern", "default": "100m", "description": "A quantity default with vCPU share." }, ...Less repetition → easier future edits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
test/basic-types/Makefile(1 hunks)test/basic-types/README.md(1 hunks)test/basic-types/values.schema.json(1 hunks)test/basic-types/values.yaml(1 hunks)test/complex-types/Makefile(1 hunks)test/complex-types/README.md(1 hunks)test/complex-types/values.schema.json(1 hunks)test/complex-types/values.yaml(1 hunks)test/lists/Makefile(1 hunks)test/lists/README.md(1 hunks)test/lists/values.schema.json(1 hunks)test/lists/values.yaml(1 hunks)test/template/Makefile(1 hunks)test/template/README.md(1 hunks)test/template/values.schema.json(1 hunks)test/template/values.yaml(1 hunks)test/test.mk(1 hunks)test/values-inside-object/README.md(1 hunks)test/values-inside-object/values.schema.json(1 hunks)test/values-inside-object/values.yaml(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
test/basic-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/template/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/complex-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/lists/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 YAMLlint (1.37.1)
test/lists/values.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
test/template/values.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
test/complex-types/values.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
[warning] 41-41: too many blank lines (1 > 0)
(empty-lines)
test/values-inside-object/values.yaml
[error] 22-22: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
test/complex-types/README.md (1)
11-19: Double-check examples vs. real defaults
quantityRequired/quantityRequiredEmptyare documented with a value ofnull/`` but the accompanyingvalues.yamlsets no value for `quantityRequired` (→ `null`) and an empty string for `quantityRequiredEmpty` (✔). Looks consistent—just ensure the README stays in sync if the YAML defaults change.test/basic-types/values.yaml (1)
20-25: Align “nullable” semantics with provided defaults
testBoolFalseNullable/testBoolTrueNullableare marked nullable yet given concrete defaults (false/true).
If the intention is optional but defaulted, drop “nullable” from the docstring; if the value can genuinely benull, the default should benullinstead.Please double-check the docblocks vs. the actual values.
| { | ||
| "title": "Chart Values", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add $schema meta-key for explicit draft version
Explicitly declaring the draft your schema adheres to avoids tooling ambiguities and validation surprises.
{
+ "$schema": "https://json-schema.org/draft-07/schema#",
"title": "Chart Values",📝 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.
| { | |
| "title": "Chart Values", | |
| { | |
| "$schema": "https://json-schema.org/draft-07/schema#", | |
| "title": "Chart Values", |
🤖 Prompt for AI Agents
In test/basic-types/values.schema.json at the beginning of the file (lines 1-2),
add the "$schema" key with the appropriate JSON Schema draft URL to explicitly
declare the schema version. This will ensure tools correctly interpret and
validate the schema according to the specified draft version.
| "testBoolFalseNullable": { | ||
| "description": "Boolean variable, defaults to false, nullable", | ||
| "type": "boolean", | ||
| "default": false | ||
| }, | ||
| "testBoolNullable": { | ||
| "description": "Boolean variable, nullable", | ||
| "type": "boolean" | ||
| }, | ||
| "testBoolTrue": { | ||
| "description": "Boolean variable, defaults to true", | ||
| "type": "boolean", | ||
| "default": true | ||
| }, | ||
| "testBoolTrueNullable": { | ||
| "description": "Boolean variable, defaults to true, nullable", | ||
| "type": "boolean", | ||
| "default": true | ||
| }, | ||
| "testInt": { | ||
| "description": "Integer variable", | ||
| "type": "integer" | ||
| }, | ||
| "testIntDefault": { | ||
| "description": "Integer variable with default value", | ||
| "type": "integer", | ||
| "default": 10 | ||
| }, | ||
| "testIntDefaultNullable": { | ||
| "description": "Integer variable with default value, nullable", | ||
| "type": "integer", | ||
| "default": 10 | ||
| }, | ||
| "testIntNullable": { | ||
| "description": "Integer variable, nullable", | ||
| "type": "integer" | ||
| }, | ||
| "testString": { | ||
| "description": "String variable", | ||
| "type": "string" | ||
| }, | ||
| "testStringDefault": { | ||
| "description": "String variable with default value", | ||
| "type": "string", | ||
| "default": "string value" | ||
| }, | ||
| "testStringDefaultNullable": { | ||
| "description": "String variable with default value, nullable", | ||
| "type": "string", | ||
| "default": "string value" | ||
| }, | ||
| "testStringEmpty": { | ||
| "description": "String variable, empty by default", | ||
| "type": "string" | ||
| }, | ||
| "testStringEmptyNullable": { | ||
| "description": "String variable, empty by default, nullable", | ||
| "type": "string" | ||
| }, | ||
| "testStringNullable": { | ||
| "description": "String variable, nullable", | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
“Nullable” properties do not actually accept null
Every property whose name includes “Nullable” is still restricted to a single primitive type.
If the intention is to allow null, the type field must be an array that includes "null" (draft-07) or use anyOf.
Example fix for one property (apply the same pattern to the others):
"testBoolFalseNullable": {
"description": "Boolean variable, defaults to false, nullable",
- "type": "boolean",
+ "type": ["boolean", "null"],
"default": false
},Without this change, null will be rejected by schema validators, defeating the test’s purpose.
📝 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.
| "testBoolFalseNullable": { | |
| "description": "Boolean variable, defaults to false, nullable", | |
| "type": "boolean", | |
| "default": false | |
| }, | |
| "testBoolNullable": { | |
| "description": "Boolean variable, nullable", | |
| "type": "boolean" | |
| }, | |
| "testBoolTrue": { | |
| "description": "Boolean variable, defaults to true", | |
| "type": "boolean", | |
| "default": true | |
| }, | |
| "testBoolTrueNullable": { | |
| "description": "Boolean variable, defaults to true, nullable", | |
| "type": "boolean", | |
| "default": true | |
| }, | |
| "testInt": { | |
| "description": "Integer variable", | |
| "type": "integer" | |
| }, | |
| "testIntDefault": { | |
| "description": "Integer variable with default value", | |
| "type": "integer", | |
| "default": 10 | |
| }, | |
| "testIntDefaultNullable": { | |
| "description": "Integer variable with default value, nullable", | |
| "type": "integer", | |
| "default": 10 | |
| }, | |
| "testIntNullable": { | |
| "description": "Integer variable, nullable", | |
| "type": "integer" | |
| }, | |
| "testString": { | |
| "description": "String variable", | |
| "type": "string" | |
| }, | |
| "testStringDefault": { | |
| "description": "String variable with default value", | |
| "type": "string", | |
| "default": "string value" | |
| }, | |
| "testStringDefaultNullable": { | |
| "description": "String variable with default value, nullable", | |
| "type": "string", | |
| "default": "string value" | |
| }, | |
| "testStringEmpty": { | |
| "description": "String variable, empty by default", | |
| "type": "string" | |
| }, | |
| "testStringEmptyNullable": { | |
| "description": "String variable, empty by default, nullable", | |
| "type": "string" | |
| }, | |
| "testStringNullable": { | |
| "description": "String variable, nullable", | |
| "type": "string" | |
| } | |
| "testBoolFalseNullable": { | |
| "description": "Boolean variable, defaults to false, nullable", | |
| "type": ["boolean", "null"], | |
| "default": false | |
| }, |
🤖 Prompt for AI Agents
In test/basic-types/values.schema.json from lines 14 to 76, properties labeled
as "Nullable" do not currently accept null values because their "type" fields
specify only a single primitive type. To fix this, update each "Nullable"
property's "type" field to be an array including the original type and "null"
(e.g., "type": ["boolean", "null"]) or use "anyOf" with the original type and a
null type schema. This change will allow these properties to accept null values
as intended.
| "quantityNullableRequiredEmpty": { | ||
| "description": "A nullable quantity value with empty string (CPU cores or RAM).", | ||
| "type": "string", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| }, |
There was a problem hiding this comment.
quantityNullableRequiredEmpty cannot be empty due to the regex
The description says “with empty string”, but the regex disallows an empty value, so "" will fail validation.
If an empty string is legitimate, make the pattern optional:
-"pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",
+"pattern": "^$|^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",📝 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.
| "quantityNullableRequiredEmpty": { | |
| "description": "A nullable quantity value with empty string (CPU cores or RAM).", | |
| "type": "string", | |
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | |
| "x-kubernetes-int-or-string": true | |
| }, | |
| "quantityNullableRequiredEmpty": { | |
| "description": "A nullable quantity value with empty string (CPU cores or RAM).", | |
| "type": "string", | |
| "pattern": "^$|^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | |
| "x-kubernetes-int-or-string": true | |
| }, |
🤖 Prompt for AI Agents
In test/complex-types/values.schema.json around lines 67 to 72, the pattern
regex for "quantityNullableRequiredEmpty" does not allow an empty string,
conflicting with the description that says it can be empty. To fix this, modify
the regex pattern to make the entire pattern optional, allowing an empty string
to pass validation while preserving the existing numeric format matching.
| "quantityRequired": { | ||
| "description": "A required quantity value (CPU cores or RAM).", | ||
| "type": "string", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| }, | ||
| "quantityRequiredEmpty": { | ||
| "description": "A required quantity value with empty string (CPU cores or RAM).", | ||
| "type": "string", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Properties labelled “Required” are not actually required
Neither quantityRequired nor quantityRequiredEmpty are listed in a top-level "required": [...] array, so they remain optional.
Add an explicit required list:
{
"title": "Chart Values",
"type": "object",
+ "required": ["quantityRequired", "quantityRequiredEmpty"],
"properties": {Add explicit required properties to JSON schema
The fields named “quantityRequired” and “quantityRequiredEmpty” aren’t enforced as required because they’re missing from the schema’s top-level required array. Please update test/complex-types/values.schema.json accordingly:
• File: test/complex-types/values.schema.json
• Location: directly after the top-level "type": "object", entry
"title": "Chart Values",
"type": "object",
+ "required": ["quantityRequired", "quantityRequiredEmpty"],
"properties": {
"quantityRequired": {
"description": "A required quantity value (CPU cores or RAM).",
"type": "string",
"pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",
"x-kubernetes-int-or-string": true
},
"quantityRequiredEmpty": {
"description": "A required quantity value with empty string (CPU cores or RAM).",
"type": "string",
"pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$",
"x-kubernetes-int-or-string": true
}This ensures both properties are truly required by the schema.
📝 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.
| "quantityRequired": { | |
| "description": "A required quantity value (CPU cores or RAM).", | |
| "type": "string", | |
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | |
| "x-kubernetes-int-or-string": true | |
| }, | |
| "quantityRequiredEmpty": { | |
| "description": "A required quantity value with empty string (CPU cores or RAM).", | |
| "type": "string", | |
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | |
| "x-kubernetes-int-or-string": true | |
| } | |
| "title": "Chart Values", | |
| "type": "object", | |
| "required": ["quantityRequired", "quantityRequiredEmpty"], | |
| "properties": { | |
| "quantityRequired": { | |
| "description": "A required quantity value (CPU cores or RAM).", | |
| "type": "string", | |
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | |
| "x-kubernetes-int-or-string": true | |
| }, | |
| "quantityRequiredEmpty": { | |
| "description": "A required quantity value with empty string (CPU cores or RAM).", | |
| "type": "string", | |
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | |
| "x-kubernetes-int-or-string": true | |
| } | |
| } |
🤖 Prompt for AI Agents
In test/complex-types/values.schema.json around lines 73 to 84, the properties
"quantityRequired" and "quantityRequiredEmpty" are defined but not enforced as
required. To fix this, add both property names to the schema's top-level
"required" array, which is located directly after the "type": "object"
declaration. This change will ensure these fields are mandatory according to the
JSON schema.
test/lists/values.schema.json
Outdated
| "test": { | ||
| "description": "Test variable" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify the expected type for test to avoid overly-permissive schema
Omitting "type" makes the property accept any JSON value, which defeats the purpose of type-focused tests.
If this directory is intended to validate lists, declare it explicitly:
"test": {
"description": "Test variable"
+ , "type": "array",
+ "items": { "type": "string" }
}Add or adjust as required for the actual data you want to enforce.
📝 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.
| "test": { | |
| "description": "Test variable" | |
| } | |
| "test": { | |
| "description": "Test variable", | |
| "type": "array", | |
| "items": { "type": "string" } | |
| } |
🤖 Prompt for AI Agents
In test/lists/values.schema.json around lines 5 to 7, the "test" property lacks
a "type" declaration, making the schema accept any JSON value and weakening type
validation. Add a "type" field to the "test" property specifying the expected
JSON data type (e.g., "string", "number", "object", etc.) according to the
intended data to enforce stricter validation.
test/test.mk
Outdated
| generate: | ||
| cozyvalues-gen -v values.yaml -s values.schema.json -r README.md | ||
|
|
||
| test: generate | ||
| git diff . No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Declare PHONY targets and make test the default rule
Without a .PHONY declaration Make may treat generate/test as real files, leading to stale-rule surprises.
Placing test (or an all) rule first communicates intent and matches conventional make entry points.
While touching the file, prefer git diff --exit-code for an explicit non-zero failure signal.
+.PHONY: generate test
+
+# default target
+all: test
+
generate:
cozyvalues-gen -v values.yaml -s values.schema.json -r README.md
-test: generate
- git diff .
+
+test: generate
+ git diff --exit-code .🤖 Prompt for AI Agents
In test/test.mk lines 1 to 5, add a .PHONY declaration for the generate and test
targets to prevent Make from confusing them with real files. Also, reorder the
rules to place test as the first (default) target. Finally, update the git diff
command in the test target to use git diff --exit-code for proper failure
signaling.
test/values-inside-object/README.md
Outdated
| | Name | Description | Type | Value | | ||
| | ----------------------------------- | ------------------------------------------- | ----------- | ------ | | ||
| | `grafana.resources.requests.cpu` | CPU resources | `*quantity` | | | ||
| | `grafana.resources.requests.memory` | Memory resources | `*quantity` | | | ||
| | `grafana` | Configuration for Grafana | `object` | `null` | | ||
| | `grafana.db` | | `object` | | | ||
| | `grafana.db.size` | Persistent Volume size for grafana database | `string` | | | ||
| | `grafana.resources` | Resources configuration for grafana | `object` | | | ||
| | `grafana.resources.limits` | Resources limits for grafana | `object` | | | ||
| | `grafana.resources.requests` | Resources requests for grafana | `object` | | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameter table inconsistencies
- Row 13 (
grafana.db) has an empty description – copy the one from the schema (“Configuration for Grafana database” or similar). - There are detailed rows for
grafana.resources.requests.*but nothing for the siblinggrafana.resources.limits.*. Add or explain their omission for symmetry. grafana.resources.requests.cpu|memorylist Type as*quantity; the preceding asterisk may confuse Markdown renderers – consider plainquantity.
🤖 Prompt for AI Agents
In test/values-inside-object/README.md around lines 8 to 17, update the
parameter table to fix inconsistencies: add a description for the `grafana.db`
row similar to "Configuration for Grafana database"; add rows for
`grafana.resources.limits.cpu` and `grafana.resources.limits.memory` with
appropriate descriptions and types to match the `requests` entries or explain
why they are omitted; and remove the asterisk from the `*quantity` type in
`grafana.resources.requests.cpu` and `grafana.resources.requests.memory` to just
`quantity` to avoid Markdown rendering issues.
| ## @section Grafana configuration | ||
|
|
||
| ## @field resources.cpu {*quantity} CPU resources | ||
| ## @field resources.memory {*quantity} Memory resources | ||
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Doc-comment paths don’t match actual structure
## @field resources.cpu & ## @field resources.memory imply top-level keys, yet they live under grafana.resources.*. Rename to avoid misleading tooling that parses these annotations.
-## @field resources.cpu {*quantity} CPU resources
-## @field resources.memory {*quantity} Memory resources
+## @field grafana.resources.*.cpu {*quantity} CPU resources
+## @field grafana.resources.*.memory {*quantity} Memory resources📝 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.
| ## @section Grafana configuration | |
| ## @field resources.cpu {*quantity} CPU resources | |
| ## @field resources.memory {*quantity} Memory resources | |
| ## @section Grafana configuration | |
| -## @field resources.cpu {*quantity} CPU resources | |
| -## @field resources.memory {*quantity} Memory resources | |
| +## @field grafana.resources.*.cpu {*quantity} CPU resources | |
| +## @field grafana.resources.*.memory {*quantity} Memory resources | |
🤖 Prompt for AI Agents
In test/values-inside-object/values.yaml at lines 1 to 6, the doc-comment paths
for resources.cpu and resources.memory incorrectly suggest they are top-level
keys, but they are actually nested under grafana.resources. Rename these
doc-comment paths to reflect the correct nested structure, such as
grafana.resources.cpu and grafana.resources.memory, to ensure accurate
documentation and tooling parsing.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
test/basic-types/Makefile (1)
1-1: Add a trailing newline for POSIX compliance
Editors and some tools expect text files to end with a newline; omitting it can introduce unnecessary diff churn.include ../test.mk +test/complex-types/Makefile (1)
1-1: Add missing trailing newline to comply with POSIX text-file conventionThe file still lacks a newline at EOF, which was flagged in the previous review. Many tools (including
git diff& POSIX utilities) expect it.include ../test.mk +test/complex-types/values.yaml (1)
6-8: Remove trailing whitespace – YAMLlint error persistsThe two spaces after the closing quotes on line 8 break linting.
-quantityRequiredEmpty: ""·· +quantityRequiredEmpty: ""test/template/Makefile (1)
1-1: Add the trailing newline that POSIX tools expect.
Same issue was noted in the previous review; keeping the file POSIX-compliant avoids spurious “No newline at end of file” warnings in diff tools and linters.include ../test.mk +test/template/values.yaml (1)
4-4: Terminate file with a newline to satisfy YAML lint.
Identical feedback was given earlier; adding the newline clears thenew-line-at-end-of-fileerror.test: +test/values-inside-object/values.yaml (2)
3-4: Doc-comment paths still point to non-existent top-level keys
Theresources.cpuandresources.memorypaths imply top-level keys, yet the actual values live undergrafana.resources.*. This will break any tooling that consumes these annotations.
22-22: Add trailing newline
File is missing the mandatory POSIX newline at EOF, already reported by YAML-lint.memory: 256Mi +test/lists/Makefile (1)
1-1: Missing trailing newline (already raised before)The file still lacks the POSIX-standard trailing newline.
Please add it to avoid tooling warnings.test/lists/values.yaml (1)
1-51: Missing trailing newline (already raised before)
yamllintstill flags the lack of a final newline. Add one to silence CI noise.test/lists/values.schema.json (1)
1-90: Missing trailing newline (already raised before)The file still ends without a newline; add one to satisfy linters.
🧹 Nitpick comments (6)
test/complex-types/values.yaml (1)
40-42: Trim superfluous blank line at file endYAMLlint warns about an extra blank line (rule
empty-lines). Deleting it keeps the file tidy.-quantityNullableDefaultRam: "500MiB" - +quantityNullableDefaultRam: "500MiB"test/template/Makefile (1)
1-1: Suppress checkmake warnings with a trivial.PHONYdeclaration (optional).
checkmakeflags the absence ofall,clean, andtestphony targets even though they are provided by the included../test.mk.
Declaring them locally silences the warning without altering behaviour.include ../test.mk +.PHONY: all clean testtest/lists/values.yaml (2)
10-14: Fix copy-paste error in description forintListMultipleDescription says “one value” but the name Multiple and example values indicate multiple items.
Align the doc-string to avoid confusion.-## @param intListMultiple {[]int} A list of integers with one value. +## @param intListMultiple {[]int} A list of integers with multiple values.
15-26: Nullable-item lists are empty → consider adding a null exampleTo exercise the “nullable element” aspect (
[]*int), add at least onenullitem to one of the_Nullable*lists; otherwise the test does not actually validate the nullable behaviour.test/lists/values.schema.json (2)
12-18: Description mis-matches parameter intent
intListMultipleis documented as “one value”; should be “multiple values” to mirrorvalues.yaml.- "description": "A list of integers with one value.", + "description": "A list of integers with multiple values.",
82-88: Optional: make “single/empty list required” really requiredIf the goal is to test presence of required keys, add a top-level
"required": [...]array so JSON-Schema enforces it. Otherwise the “required” wording in docs is misleading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
examples/virtual-machine.yaml(1 hunks)test/basic-types/Makefile(1 hunks)test/basic-types/README.md(1 hunks)test/basic-types/values.schema.json(1 hunks)test/basic-types/values.yaml(1 hunks)test/complex-types/Makefile(1 hunks)test/complex-types/README.md(1 hunks)test/complex-types/values.schema.json(1 hunks)test/complex-types/values.yaml(1 hunks)test/lists/Makefile(1 hunks)test/lists/README.md(1 hunks)test/lists/values.schema.json(1 hunks)test/lists/values.yaml(1 hunks)test/template/Makefile(1 hunks)test/template/README.md(1 hunks)test/template/values.schema.json(1 hunks)test/template/values.yaml(1 hunks)test/test.mk(1 hunks)test/values-inside-object/README.md(1 hunks)test/values-inside-object/values.schema.json(1 hunks)test/values-inside-object/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- examples/virtual-machine.yaml
- test/basic-types/README.md
- test/basic-types/values.yaml
- test/complex-types/values.schema.json
🚧 Files skipped from review as they are similar to previous changes (8)
- test/template/README.md
- test/template/values.schema.json
- test/test.mk
- test/lists/README.md
- test/basic-types/values.schema.json
- test/values-inside-object/README.md
- test/complex-types/README.md
- test/values-inside-object/values.schema.json
🧰 Additional context used
🪛 YAMLlint (1.37.1)
test/lists/values.yaml
[error] 51-51: no new line character at the end of file
(new-line-at-end-of-file)
test/complex-types/values.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
[warning] 41-41: too many blank lines (1 > 0)
(empty-lines)
test/template/values.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
test/values-inside-object/values.yaml
[error] 22-22: no new line character at the end of file
(new-line-at-end-of-file)
🪛 checkmake (0.2.2)
test/basic-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/complex-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/lists/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/template/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🔇 Additional comments (2)
test/basic-types/Makefile (1)
1-1: Confirm.PHONYtargets are provided by../test.mk
Static analysis reports missingall,clean, andtestphony targets. If../test.mkdoes not declare them, add explicit.PHONYdeclarations here; otherwise, suppress the warning in the linter config.test/lists/Makefile (1)
1-1:checkmakewarnings are false positives here
all,clean, andtestphony targets are provided by../test.mk, so it is OK that they are not re-declared locally. You can safely ignore those specificcheckmakehints for this minimal include-only stub.
| "intListNullable": { | ||
| "description": "A nullable list of integers, empty.", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "integer" | ||
| } | ||
| }, | ||
| "intListNullableMultiple": { | ||
| "description": "A nullable list of integers with multiple values.", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "integer" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
*_Nullable* lists allow null items – but schema forbids them
For lists whose Go-style type is []*int, each item can be null.
Add null to the item type union so validation matches the spec.
- "items": {
- "type": "integer"
- }
+ "items": {
+ "type": ["integer", "null"]
+ }Apply the same change to:
intListNullableMultipleintListNullableSingle
📝 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.
| "intListNullable": { | |
| "description": "A nullable list of integers, empty.", | |
| "type": "array", | |
| "items": { | |
| "type": "integer" | |
| } | |
| }, | |
| "intListNullableMultiple": { | |
| "description": "A nullable list of integers with multiple values.", | |
| "type": "array", | |
| "items": { | |
| "type": "integer" | |
| } | |
| }, | |
| "intListNullable": { | |
| "description": "A nullable list of integers, empty.", | |
| "type": "array", | |
| - "items": { | |
| - "type": "integer" | |
| - } | |
| + "items": { | |
| + "type": ["integer", "null"] | |
| + } | |
| }, | |
| "intListNullableMultiple": { | |
| "description": "A nullable list of integers with multiple values.", | |
| "type": "array", | |
| - "items": { | |
| - "type": "integer" | |
| - } | |
| + "items": { | |
| + "type": ["integer", "null"] | |
| + } | |
| }, |
🤖 Prompt for AI Agents
In test/lists/values.schema.json around lines 19 to 32, the schemas for
intListNullable, intListNullableMultiple, and intListNullableSingle define items
as only integers, but they should allow null values as well to match the Go type
[]*int. Update the "items" type to be a union of "integer" and "null" for these
lists so that each item can be either an integer or null, ensuring the schema
validation aligns with the specification.
| "stringListNullable": { | ||
| "description": "A nullable list of strings, empty.", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "stringListNullableMultiple": { | ||
| "description": "A nullable list of strings with multiple values.", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same nullable-item issue for string lists
Update the items.type for
stringListNullablestringListNullableMultiplestringListNullableSingle
to accept both "string" and null, mirroring the integer lists.
🤖 Prompt for AI Agents
In test/lists/values.schema.json between lines 61 and 74, the string list
schemas stringListNullable, stringListNullableMultiple, and
stringListNullableSingle have items.type set only to "string". Update these to
accept both "string" and "null" by changing items.type to an array containing
"string" and "null", matching the nullable integer list pattern.
| ## @field grafana.resources {*grafanaResources} Resources configuration for grafana | ||
| ## @field grafanaResources.limits {*resources} Resources limits for grafana | ||
| ## @field grafanaResources.requests {*resources} Resources requests for grafana |
There was a problem hiding this comment.
Inconsistent prefix: grafanaResources.* should be grafana.resources.*
The two annotations introduce a new pseudo-root (grafanaResources) that doesn’t exist in the YAML, leading to misleading documentation and failed schema generation.
-## @field grafanaResources.limits {*resources} Resources limits for grafana
-## @field grafanaResources.requests {*resources} Resources requests for grafana
+## @field grafana.resources.limits {*resources} Resources limits for grafana
+## @field grafana.resources.requests {*resources} Resources requests for grafana📝 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.
| ## @field grafana.resources {*grafanaResources} Resources configuration for grafana | |
| ## @field grafanaResources.limits {*resources} Resources limits for grafana | |
| ## @field grafanaResources.requests {*resources} Resources requests for grafana | |
| ## @field grafana.resources {*grafanaResources} Resources configuration for grafana | |
| ## @field grafana.resources.limits {*resources} Resources limits for grafana | |
| ## @field grafana.resources.requests {*resources} Resources requests for grafana |
🤖 Prompt for AI Agents
In test/values-inside-object/values.yaml around lines 10 to 12, the annotations
use an inconsistent prefix `grafanaResources.*` which does not match the actual
YAML structure. Update the annotations to use the correct prefix
`grafana.resources.*` to align with the YAML keys, ensuring accurate
documentation and successful schema generation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
test/basic-types/Makefile (1)
1-1: Add a trailing newline to comply with POSIX text-file conventionThe file still lacks a terminating newline; many tools warn or mis-handle files without it.
-include ../test.mk +include ../test.mk +test/template/Makefile (1)
1-1: Add trailing newline to comply with POSIX & avoid tooling noise.The file is missing a terminating newline character, which several tools (e.g.,
git diff, editors, linters) expect.
Adding it avoids spurious warnings and keeps consistency with other Makefiles.include ../test.mk +test/template/values.yaml (1)
1-4: Terminate file with a newline to satisfy YAML-lint.YAML-lint flags the missing newline at EOF. Adding it silences the error and aligns with project style.
test: +test/lists/Makefile (1)
1-1: Add trailing newline to comply with POSIX text-file convention
The file lacks a terminating newline character. While harmless for Make, some tools and git diff viewers flag this as a style error.include ../test.mk +test/complex-types/Makefile (1)
1-1: Missing trailing newline – breaks POSIX text-file conventionThe file still lacks a terminating LF, which some tools (
cat,git diff,makeon BSD) treat as an anomaly.
Add a final blank line.test/complex-types/values.yaml (1)
7-9: Trailing spaces break YAML-lintWhitespace remains after the empty string on Line 8, re-introducing the lint error flagged earlier.
-quantityRequiredEmpty: ""·· +quantityRequiredEmpty: ""
🧹 Nitpick comments (3)
test/lists/values.yaml (1)
50-51: Terminate file with a newline
YAML-lint flagsno new line character at the end of file. Add a single\nafter the last line to silence the linter and follow standard text-file etiquette.- "user2" +test/complex-types/Makefile (1)
1-1: checkmake warnings can be safely ignored here
all / clean / testphony targets are provided by the sharedtest.mk. Duplicating them locally would only create maintenance noise.test/complex-types/values.yaml (1)
21-22: Superfluous blank lines—trim to satisfy lint rulesYAML-lint warns about extra blank lines at Lines 22 and 42. Delete them to keep a single blank line between logical sections and none at EOF.
- ## @section Nullable quantity parameters @@ -quantityNullableDefaultRam: "500MiB" - +quantityNullableDefaultRam: "500MiB"Also applies to: 41-42
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
examples/virtual-machine.yaml(1 hunks)test/basic-types/Makefile(1 hunks)test/basic-types/README.md(1 hunks)test/basic-types/values.schema.json(1 hunks)test/basic-types/values.yaml(1 hunks)test/complex-types/Makefile(1 hunks)test/complex-types/README.md(1 hunks)test/complex-types/values.schema.json(1 hunks)test/complex-types/values.yaml(1 hunks)test/fields/README.md(1 hunks)test/fields/values.schema.json(1 hunks)test/fields/values.yaml(1 hunks)test/lists/Makefile(1 hunks)test/lists/README.md(1 hunks)test/lists/values.schema.json(1 hunks)test/lists/values.yaml(1 hunks)test/template/Makefile(1 hunks)test/template/README.md(1 hunks)test/template/values.schema.json(1 hunks)test/template/values.yaml(1 hunks)test/test.mk(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- test/fields/README.md
- test/fields/values.yaml
- test/basic-types/values.yaml
- test/fields/values.schema.json
- test/complex-types/README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- examples/virtual-machine.yaml
- test/template/values.schema.json
- test/test.mk
- test/template/README.md
- test/lists/values.schema.json
- test/lists/README.md
- test/basic-types/values.schema.json
- test/basic-types/README.md
- test/complex-types/values.schema.json
🧰 Additional context used
🪛 checkmake (0.2.2)
test/basic-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/complex-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/lists/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/template/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 YAMLlint (1.37.1)
test/complex-types/values.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
[warning] 41-41: too many blank lines (1 > 0)
(empty-lines)
test/lists/values.yaml
[error] 51-51: no new line character at the end of file
(new-line-at-end-of-file)
test/template/values.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
test/template/Makefile (1)
1-1: checkmake warnings can be safely ignored here
all,clean, andtestPHONY targets are provided by the included../test.mk. Declaring them again locally would be redundant.
No action required unless the include path ever changes.
Signed-off-by: Nick Volynkin <nick.volynkin@gmail.com>
NickVolynkin
force-pushed
the
tests
branch
2 times, most recently
from
9637d32 to
9cb2f5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
test/template/Makefile (1)
1-1: Add trailing newline to conform with POSIX text-file convention
The file currently lacks a terminating newline, which is required by POSIX and flagged by multiple linters.include ../test.mk +test/template/values.yaml (1)
4-4: Terminate file with a newline
The final line is missing a newline character, still triggering YAML-lint’snew-line-at-end-of-fileerror.test: +test/basic-types/Makefile (1)
1-1: Add a trailing newline at EOF
POSIX text files conventionally end with a newline; keeping the convention avoids tooling-related diffs.test/lists/Makefile (1)
1-1: Add trailing newline to satisfy POSIX text-file convention
Same feedback as in a previous review: terminate the file with a single\n.test/lists/values.yaml (2)
10-13: Comment describes “one value” but the list contains two
Update the description to reflect multiple elements.-## @param intListMultiple {[]int} A list of integers with one value. +## @param intListMultiple {[]int} A list of integers with multiple values.
51-51: Terminate file with a newline
YAML-lint flags the missing trailing newline; add it to keep tooling quiet.- "user2" +test/complex-types/Makefile (1)
1-1: Add the missing trailing newline to satisfy POSIX text-file conventionSame comment as in the previous review – still unresolved.
-include ../test.mk +include ../test.mk +test/complex-types/values.yaml (1)
7-9: Remove stray whitespace line to pass YAML lintThe blank line at Line 8 consists solely of spaces and triggers the “trailing-spaces” error.
-quantityRequiredEmpty: "" - +quantityRequiredEmpty: ""
🧹 Nitpick comments (5)
test/template/values.yaml (1)
4-4: Consider making the intent explicit
Iftestis meant to be an empty object, defining it as{}avoids ambiguity between “empty map” and “null/unset”, and plays nicer with some schema validators.- test: + test: {}test/basic-types/Makefile (1)
1-1: Silence checkmake by declaring phony targets locally
checkmakecomplains about missingall/clean/testtargets because it doesn’t follow theinclude. A minimal, non-intrusive fix is to add phony declarations here so the linter is satisfied while the real recipes stay intest.mk.include ../test.mk + +.PHONY: all clean testIf you prefer not to duplicate declarations, consider configuring
checkmaketo followincludedirectives instead. Let me know which approach you’d like and I can supply the exactcheckmakeconfig snippet.test/complex-types/values.yaml (1)
39-42: Drop superfluous blank line at EOFYAMLlint warns about “too many blank lines”. Keep a single terminating newline only.
-quantityNullableDefaultRam: "500MiB" - +quantityNullableDefaultRam: "500MiB"test/fields/values.schema.json (2)
29-33: Missing default forbar.db.sizecreates asymmetric behaviour vsfoo.db.size
foo.db.sizeexplicitly carries"default": "10Gi"(Line 61-63) whereasbar.db.sizedoes not. Either remove the default fromfooor add the same default here to keep behaviour predictable.
1-68: Consider disallowing undeclared top-level keys with"additionalProperties": falseUnless the chart intentionally supports arbitrary keys, add
"additionalProperties": falseat each object level to catch misspellings early.No diff provided because the change touches many blocks; apply where appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
examples/virtual-machine.yaml(1 hunks)test/basic-types/Makefile(1 hunks)test/basic-types/README.md(1 hunks)test/basic-types/values.schema.json(1 hunks)test/basic-types/values.yaml(1 hunks)test/complex-types/Makefile(1 hunks)test/complex-types/README.md(1 hunks)test/complex-types/values.schema.json(1 hunks)test/complex-types/values.yaml(1 hunks)test/fields/Makefile(1 hunks)test/fields/README.md(1 hunks)test/fields/values.schema.json(1 hunks)test/fields/values.yaml(1 hunks)test/lists/Makefile(1 hunks)test/lists/README.md(1 hunks)test/lists/values.schema.json(1 hunks)test/lists/values.yaml(1 hunks)test/template/Makefile(1 hunks)test/template/README.md(1 hunks)test/template/values.schema.json(1 hunks)test/template/values.yaml(1 hunks)test/test.mk(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/basic-types/README.md
🚧 Files skipped from review as they are similar to previous changes (12)
- examples/virtual-machine.yaml
- test/template/README.md
- test/test.mk
- test/template/values.schema.json
- test/complex-types/README.md
- test/basic-types/values.schema.json
- test/lists/values.schema.json
- test/lists/README.md
- test/fields/values.yaml
- test/fields/README.md
- test/complex-types/values.schema.json
- test/basic-types/values.yaml
🧰 Additional context used
🪛 checkmake (0.2.2)
test/basic-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/complex-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/fields/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/lists/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/template/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 YAMLlint (1.37.1)
test/complex-types/values.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
[warning] 41-41: too many blank lines (1 > 0)
(empty-lines)
test/lists/values.yaml
[error] 51-51: no new line character at the end of file
(new-line-at-end-of-file)
test/template/values.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
test/fields/Makefile (1)
1-1: Static-analysis warnings are false positives here
checkmakecomplains about missingall/clean/testphony targets, but those targets are already provided by the included../test.mk. No action required.
| ## @param bar {bar} Configuration for bar | ||
| ## @field bar.db {barDB} Field with custom type declared locally | ||
| ## @field barDB.size {string} Sub-field declared with path relative to custom type | ||
| ## @field bar.db.volume {string} Sub-field declared with absolute path |
There was a problem hiding this comment.
$ cozyvalues-gen -v values.yaml -s values.schema.json -r README.md
write JSON schema: values.schema.json
README: validate values: field 'bar.db.volume' is not defined in schema
make: *** [generate] Error 1There was a problem hiding this comment.
If declarations are mixed, generation fails. Schema is updated, readme is not updated.
| "quantityNullable": { | ||
| "description": "A nullable quantity value.", | ||
| "type": "string", | ||
| "pattern": "^(\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\\+|-)?(([0-9]+(\\.[0-9]*)?)|(\\.[0-9]+))))?$", | ||
| "x-kubernetes-int-or-string": true | ||
| }, |
There was a problem hiding this comment.
@kvaps is this how nullable types are declared for K8s API?
| | ------------------------ | --------------------------------------------- | ------ | ------ | | ||
| | `testInt` | Integer variable | `int` | `0` | | ||
| | `testIntDefault` | Integer variable with default value | `int` | `10` | | ||
| | `testIntNullable` | Integer variable, nullable | `*int` | `null` | |
There was a problem hiding this comment.
This is correct behavior, because if type is nullabe it can be unset, we should not provide any empty values for it
| | `testBool` | Boolean variable | `bool` | `false` | | ||
| | `testBoolFalse` | Boolean variable, defaults to false | `bool` | `false` | | ||
| | `testBoolTrue` | Boolean variable, defaults to true | `bool` | `true` | | ||
| | `testBoolNullable` | Boolean variable, nullable | `*bool` | `null` | |
There was a problem hiding this comment.
This is correct behavior, because if type is nullabe it can be unset, we should not provide any empty values for it
| | `testString` | String variable | `string` | `""` | | ||
| | `testStringEmpty` | String variable, empty by default | `string` | `""` | | ||
| | `testStringDefault` | String variable with default value | `string` | `string value` | | ||
| | `testStringNullable` | String variable, nullable | `*string` | `null` | |
There was a problem hiding this comment.
This is correct behavior, because if type is nullabe it can be unset, we should not provide any empty values for it
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/complex-types/Makefile (1)
1-2: Add trailing newline to conform with POSIX text-file conventionSeveral linters (including
git diff --check) will flag the missing terminating newline. Add a final blank line to pre-empt recurring noise in reviews/CI.test/lists/Makefile (1)
1-1: Add a trailing newline at EOFPOSIX text files must end with a newline. It also prevents diff-related edge-cases when appending further lines.
include ../test.mk +test/basic-types/Makefile (1)
1-2: Still missing the final newlineThe earlier review already highlighted the POSIX convention of ending text files with a newline. Please append it here as well.
🧹 Nitpick comments (4)
test/complex-types/Makefile (1)
1-1: Optionally declare dummy phony targets to appeasecheckmake
checkmakewarns about missingall,clean, andtestphony targets because it only parses this file and doesn’t followinclude. Either (a) ignore the warning in CI or (b) add stubs that forward to the included rules:include ../test.mk +.PHONY: all clean test +all clean test: + @$(MAKE) -f ../test.mk $@Purely cosmetic—choose based on your lint policy.
test/basic-types/Makefile (1)
1-1: Explicit.PHONYshim keepscheckmakequiet
checkmakedoesn’t follow theincludedirective, so it flags missingall,clean, andtesttargets. Adding a no-op shim avoids false positives without duplicating the real targets intest.mk:.PHONY: all clean test all clean test: @:test/fields/Makefile (1)
1-1: Add local.PHONYstubs or suppress checkmake warnings
checkmakeflags the absence ofall,clean, andtesttargets even though they are very likely provided by../test.mk.
To keep the root Makefile clean and silence the linter, either:+.PHONY: all clean test +all clean test: + @:or add a directive such as:
# checkmake: ignore minphonyat the top of this file.
Choose whichever best matches your repo’s style.test/fields/values.schema.json (1)
1-4: Embed the$schemameta-field for full JSON-Schema compatibilityMany tooling chains rely on the
$schemaURI to pick the correct draft and capabilities. Insert it right after the opening brace:{ + "$schema": "https://json-schema.org/draft-07/schema#", "title": "Chart Values", "type": "object",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
test/basic-types/Makefile(1 hunks)test/basic-types/README.md(1 hunks)test/complex-types/Makefile(1 hunks)test/complex-types/README.md(1 hunks)test/fields/Makefile(1 hunks)test/fields/README.md(1 hunks)test/fields/values.schema.json(1 hunks)test/fields/values.yaml(1 hunks)test/lists/Makefile(1 hunks)test/lists/README.md(1 hunks)test/lists/values.schema.json(1 hunks)test/template/Makefile(1 hunks)test/template/README.md(1 hunks)test/template/values.schema.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fields/values.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- test/template/README.md
- test/complex-types/README.md
- test/lists/values.schema.json
- test/basic-types/README.md
- test/lists/README.md
- test/template/values.schema.json
- test/fields/README.md
🧰 Additional context used
🪛 checkmake (0.2.2)
test/basic-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/complex-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/fields/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/lists/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/template/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🔇 Additional comments (3)
test/template/Makefile (1)
1-2: Minimal include-only Makefile is fine; ignore checkmake “missing phony target” warningsAll required targets are presumably provided by
../test.mk, so the local file doesn’t need to redeclare them. The checkmake warnings are therefore false positives and can be safely ignored.test/lists/Makefile (1)
1-1: Confirmcheckmakephony-target warnings
checkmakeflags the absence ofall,clean, andtestphony targets. If these are defined inside../test.mk, consider silencing the warning with an inline# checkmake: ignorecomment; otherwise add small stubs to keep local tooling quiet.test/fields/values.schema.json (1)
30-38: Volume property is now present – issue resolved ✅The previously-reported omission of
volumeunderbar.db.propertieshas been fixed. The schema now fully describes the defaults, ensuring downstream validators and UI generators will recognise the field.
| | `stringList` | A required list of strings, empty. | `[]string` | `[]` | | ||
| | `stringListSingle` | A required list of strings with one value. | `[]string` | `[user1]` | | ||
| | `stringListMultiple` | A required list of strings with multiple values. | `[]string` | `[user1, user2]` | | ||
| | `stringListNullable` | A nullable list of strings, empty. | `*[]string` | `null` | |
There was a problem hiding this comment.
@kvaps if I declare nullable list correctly, empty value is null, must be []
There was a problem hiding this comment.
This is correct behavior, because if type is nullabe it can be unset, we should not provide any empty values for it
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/complex-types/Makefile (1)
1-2: Add a trailing newline to satisfy POSIX text-file conventionSame issue was pointed out previously: the file still has no terminating newline, which most editors and CI linters expect.
include ../test.mk +test/lists/values.yaml (2)
10-13: Fix docstring – list actually contains multiple values
The comment says “one value” but the list has two elements.-## @param intListMultiple {[]int} A list of integers with one value. +## @param intListMultiple {[]int} A list of integers with multiple values.
62-63: Add trailing newline at end of filePOSIX text files must end with a newline. Please append one after the last line.
🧹 Nitpick comments (1)
test/lists/Makefile (1)
1-2: Consider declaring mandatory phony targets or suppressing checkmake warnings
checkmakewarns about missingall,clean, andtestphony targets.
If../test.mkalready defines them, silence the warning by adding explicit.PHONYdeclarations here, or configurecheckmaketo ignore this file.+.PHONY: all clean test + include ../test.mk
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
test/basic-types/Makefile(1 hunks)test/basic-types/README.md(1 hunks)test/complex-types/Makefile(1 hunks)test/complex-types/README.md(1 hunks)test/fields/Makefile(1 hunks)test/fields/README.md(1 hunks)test/fields/values.schema.json(1 hunks)test/fields/values.yaml(1 hunks)test/lists/Makefile(1 hunks)test/lists/README.md(1 hunks)test/lists/values.schema.json(1 hunks)test/lists/values.yaml(1 hunks)test/template/Makefile(1 hunks)test/template/README.md(1 hunks)test/template/values.schema.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/basic-types/README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- test/template/README.md
- test/fields/README.md
- test/template/values.schema.json
- test/lists/values.schema.json
- test/fields/values.yaml
- test/complex-types/README.md
- test/lists/README.md
- test/fields/values.schema.json
🧰 Additional context used
🪛 checkmake (0.2.2)
test/basic-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/complex-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/fields/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/lists/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/template/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 YAMLlint (1.37.1)
test/lists/values.yaml
[error] 63-63: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (3)
test/template/Makefile (1)
1-2: No changes needed – inclusion is correct
The Makefile cleanly delegates all targets to../test.mk; the checkmake warnings are expected and benign because the required phony targets live in the included file.test/fields/Makefile (1)
1-1: Confirm.PHONYtargets are exposed throughtest.mk
checkmakeflags the absence ofall,clean, andtestphony targets in this file.
If those targets are indeed defined in../test.mk, no action is required and you might want to configurecheckmaketo followincludedirectives.If they are not, add them here to satisfy both the build and static-analysis expectations:
include ../test.mk +.PHONY: all clean testtest/basic-types/Makefile (1)
1-2: Minimal wrapper Makefile is fine; earlier EOF-newline issue resolvedThe file now ends with a newline and simply delegates all targets to
../test.mk, which is an acceptable pattern; no further action required.
NickVolynkin
force-pushed
the
tests
branch
4 times, most recently
from
d164039 to
8f722da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/basic-types/Makefile (1)
1-2: Ensure the file ends with a trailing newlineThis was noted in a previous review; if the newline is still missing, please add one to comply with POSIX text-file conventions.
test/lists/Makefile (1)
1-2: Add the missing trailing newline
The file still lacks the customary trailing newline, which many tools (git diff, POSIX text-file rules, linters) expect.
🧹 Nitpick comments (2)
test/fields/Makefile (1)
1-1: Optionally silence false-positive checkmake warnings
checkmakecomplains about missingall,clean, andtesttargets because it doesn’t parse the included file. If these targets are indeed defined in../test.mk, consider one of:+# checkmake: disable=all # targets live in ../test.mk include ../test.mkor adding a very small stub that simply forwards to the included targets to keep static analysis green.
test/lists/values.yaml (1)
91-91: Insert a trailing newline
YAML-lint flags the missing newline at EOF. Adding it prevents pointless diffs and keeps linters quiet.- buzz: 2GiB \ No newline at end of file + buzz: 2GiB +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
test/basic-types/Makefile(1 hunks)test/basic-types/README.md(1 hunks)test/complex-types/Makefile(1 hunks)test/complex-types/README.md(1 hunks)test/complex-types/values.schema.json(1 hunks)test/complex-types/values.yaml(1 hunks)test/fields/Makefile(1 hunks)test/fields/README.md(1 hunks)test/fields/values.schema.json(1 hunks)test/fields/values.yaml(1 hunks)test/lists/Makefile(1 hunks)test/lists/README.md(1 hunks)test/lists/values.schema.json(1 hunks)test/lists/values.yaml(1 hunks)test/template/Makefile(1 hunks)test/template/README.md(1 hunks)test/template/values.schema.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/basic-types/README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- test/fields/values.yaml
- test/complex-types/values.schema.json
- test/lists/README.md
- test/fields/values.schema.json
- test/template/values.schema.json
- test/template/README.md
- test/fields/README.md
- test/complex-types/README.md
- test/lists/values.schema.json
🧰 Additional context used
🪛 checkmake (0.2.2)
test/basic-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/complex-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/fields/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/lists/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/template/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 YAMLlint (1.37.1)
test/complex-types/values.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
test/lists/values.yaml
[error] 91-91: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (5)
test/basic-types/Makefile (1)
1-1: Single-line Makefile is sufficient; no further targets needed here.The file just delegates to the shared
test.mk, so the warnings fromcheckmakeabout missingall/clean/testPHONY targets are a false-positive—they are provided by the included makefile. No action required.test/fields/Makefile (1)
1-1: Centralised test rule inclusion is sound
The Makefile mirrors the pattern used in othertest/*directories, delegating all targets to../test.mk. This keeps the repo DRY and makes future changes to the test harness trivial.test/template/Makefile (1)
1-2: Fine as-is – shared rules come fromtest.mk.Including
../test.mkcentralises all phony targets, so the local file doesn’t need to repeat them. The checkmake warnings can be safely ignored or suppressed.test/complex-types/Makefile (1)
1-2: Makefile looks fine – inclusion only, newline now presentThe file now ends with a newline and simply delegates to
../test.mk. The missing-phony warnings fromcheckmakeare a false positive because the targets are defined in the included file. Nothing to change.test/complex-types/values.yaml (1)
18-20: Verify quantity unit “MiB” vs. Kubernetes-standard “Mi”
"500MiB"is not accepted by Kubernetesresource.Quantity(the canonical suffix isMi). If this test is intended for Helm/K8s quantity validation, consider using"500Mi".
| ## @param quantityRequiredEmpty {quantity} A required quantity value with empty string (CPU cores or RAM). | ||
| quantityRequiredEmpty: "" | ||
|
|
||
| ## @param quantityDefaultInt {quantity} A quantity default with a bare integer. |
There was a problem hiding this comment.
Remove trailing spaces – YAML lint still failing
Line 8 ends with two stray spaces:
quantityRequiredEmpty: ""··These spaces trip YAMLlint’s trailing-spaces rule and will break CI on strict configurations.
-quantityRequiredEmpty: ""··
+quantityRequiredEmpty: ""🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 8-8: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In test/complex-types/values.yaml around lines 6 to 9, remove the two trailing
spaces at the end of line 8 after the empty string value for
quantityRequiredEmpty. Ensure there are no spaces after the closing quotes to
comply with YAML lint rules and prevent CI failures.
| ## @param intList {[]int} A required list of integers, empty. | ||
| intList: | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
empty vs null – value does not match the description
For each of the “empty list” parameters the YAML currently sets the value to null (key: with no value) rather than an empty list (key: []). This breaks the stated contract in the doc-comment (A … list … empty) and can lead to schema validation failures or subtle bugs in templates that distinguish empty from nil.
-intList:
+intList: []
-# … same pattern applies to …
-intListNullable:
+intListNullable: []
-stringList:
+stringList: []
-stringListNullable:
+stringListNullable: []
-stringListNullable2:
+stringListNullable2: []Apply the same fix to every “empty list” variant to keep docs and data consistent.
Also applies to: 15-17, 29-31, 41-43, 53-55
🤖 Prompt for AI Agents
In test/lists/values.yaml at lines 3 to 5 and similarly at lines 15-17, 29-31,
41-43, and 53-55, the parameters described as empty lists are currently set to
null (key with no value). Update these entries to explicitly use empty list
syntax (key: []) to match the documentation and avoid schema validation issues
or template bugs.
fb3d87f to
8dc3563
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
test/basic-types/Makefile (1)
1-2: Missing trailing newline at EOF
The file still lacks the mandatory trailing newline that POSIX utilities expect.test/complex-types/Makefile (1)
1-2: Add a trailing newline to comply with POSIX text-file conventionThe file still lacks a terminating newline, which several editors, CI linters, and diff tools expect.
This was already pointed out earlier.include ../test.mk +test/complex-types/values.yaml (1)
6-9: Remove trailing spaces on the blank line belowquantityRequiredEmptyLine 8 contains two stray spaces and violates YAMLlint’s
trailing-spacesrule, keeping CI red.- +test/lists/values.yaml (1)
3-26: Empty-list parameters are assignednull, breaking the stated contractFor every parameter whose docstring says “empty list”, the YAML leaves the value blank (
key:), which deserialises to null rather than[].
This contradicts the declared types ([]int,[]*int,[]string,*[]string,[]*string) and will fail schema validation or template logic that distinguishes empty from nil.-intList: +intList: [] -intListNullable: +intListNullable: [] -stringList: +stringList: [] -stringListNullable: +stringListNullable: [] -stringListNullable2: +stringListNullable2: [](Apply the same pattern to every “empty” variant in this file.)
Also applies to: 29-55
🧹 Nitpick comments (1)
test/lists/values.yaml (1)
90-91: Missing terminating newlineYAML-lint flags the absence of a final newline character.
Add a single\nat the end of the file to respect POSIX text-file conventions and silence linters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
test/basic-types/Makefile(1 hunks)test/basic-types/README.md(1 hunks)test/complex-types/Makefile(1 hunks)test/complex-types/README.md(1 hunks)test/complex-types/values.schema.json(1 hunks)test/complex-types/values.yaml(1 hunks)test/lists/Makefile(1 hunks)test/lists/README.md(1 hunks)test/lists/values.schema.json(1 hunks)test/lists/values.yaml(1 hunks)test/objects/Makefile(1 hunks)test/objects/README.md(1 hunks)test/objects/values.schema.json(1 hunks)test/objects/values.yaml(1 hunks)test/template/Makefile(1 hunks)test/template/README.md(1 hunks)test/template/values.schema.json(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- test/objects/README.md
- test/complex-types/values.schema.json
- test/objects/values.schema.json
- test/objects/values.yaml
- test/basic-types/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- test/template/README.md
- test/lists/README.md
- test/complex-types/README.md
- test/lists/values.schema.json
- test/template/values.schema.json
🧰 Additional context used
🪛 checkmake (0.2.2)
test/basic-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/complex-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/lists/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/objects/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/template/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 YAMLlint (1.37.1)
test/complex-types/values.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
test/lists/values.yaml
[error] 91-91: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (4)
test/template/Makefile (1)
1-1: Shared include looks good – no action requiredDelegating to
../test.mkkeeps per-directory Makefiles minimal while inheriting the commonall / clean / testphony targets, so the checkmake warnings are expected false positives.test/basic-types/Makefile (1)
1-1: Confirm that required.PHONYtargets are provided bytest.mk
checkmakeflags missingall,clean, andtestphony targets. If../test.mkalready defines them, nothing to do. Otherwise, add them here or intest.mk.test/objects/Makefile (1)
1-1: Static-analysis warnings are probably a false alarm – verifytest.mkexportsall/clean/test.
checkmakecomplains about missing phony targets, but it does not followincludedirectives.
Please double-check thattest/test.mkindeed defines theall,clean, andtesttargets.
If it does, feel free to ignore the warning or suppress it with local stubs:.PHONY: all clean testOtherwise add the targets here to prevent CI noise.
test/lists/Makefile (1)
1-2: No issues – include statement is sufficient
test.mkcentralises the PHONY targets, so this stub Makefile correctly delegates to the shared logic.
Nothing to change here.
Signed-off-by: Nick Volynkin <nick.volynkin@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
test/complex-types/values.yaml (1)
7-8: Remove trailing spaces – YAML-lint still failsTwo stray spaces remain at the end of the line, tripping
trailing-spaceslint rules.-quantityRequiredEmpty: ""·· +quantityRequiredEmpty: ""test/lists/Makefile (1)
1-2: Ensure trailing newline at end of fileThe file still lacks the mandatory newline character at EOF, which is a POSIX/Text-file convention and keeps tools like
git diffand some linters quiet.include ../test.mk +test/lists/values.yaml (2)
3-5:intListmarked “empty” but is serialised asnull
intListis documented as a required, empty[]int, yet omitting the value serialises it tonull.
For a non-nullable list the empty value must be[].-intList: +intList: []
29-31:stringListhas the same empty-vs-null mismatchAs with
intList, this required list should be an empty array, notnull.-stringList: +stringList: []
🧹 Nitpick comments (4)
test/objects/Makefile (1)
1-2: Relative include works, but consider making it location-independent
include ../test.mkis perfectly fine as long as the directory layout is stable.
If future refactors move thisMakefile, the relative path will break.
A robust pattern commonly used in sub-Makefiles is:include $(dir $(lastword $(MAKEFILE_LIST)))../test.mkwhich always resolves the path relative to the current file, not the shell’s working directory.
No action required if the tree is stable, but worth considering for long-term resilience.
(checkmake’s all/clean/test warnings are false positives here because those targets exist intest.mk.)test/basic-types/Makefile (1)
1-1: Add local.PHONYdeclarations to silencecheckmakenoise
checkmakeanalyses this file in isolation and therefore can’t see the targets provided by../test.mk, resulting in the “missing phony target” warnings. A minimalist fix is to declare them locally:include ../test.mk + +.PHONY: all clean testBehaviour stays unchanged while the linter quiets down.
test/complex-types/Makefile (1)
1-2: Minimal include-only Makefile is acceptable, but consider silencingcheckmakewarnings
checkmakecomplains about missingall/clean/testphony targets.
Because this Makefile only delegates to../test.mk, those targets will already be provided, so functionally nothing is wrong.
If you want a clean CI signal, add stub.PHONYdeclarations here or suppress the rule in the linter config.test/lists/values.yaml (1)
91-91: Add a final newlineYAML-lint flags the missing newline at EOF. Add one to silence tooling.
- name: "example 2 - not expected to appear in the README" foo: fizz: 10 buzz: 1GiB bar: fizz: 20 buzz: 2GiB +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
test/basic-types/Makefile(1 hunks)test/basic-types/README.md(1 hunks)test/complex-types/Makefile(1 hunks)test/complex-types/README.md(1 hunks)test/complex-types/values.schema.json(1 hunks)test/complex-types/values.yaml(1 hunks)test/lists/Makefile(1 hunks)test/lists/README.md(1 hunks)test/lists/values.schema.json(1 hunks)test/lists/values.yaml(1 hunks)test/objects/Makefile(1 hunks)test/objects/README.md(1 hunks)test/objects/values.schema.json(1 hunks)test/objects/values.yaml(1 hunks)test/template/Makefile(1 hunks)test/template/README.md(1 hunks)test/template/values.schema.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- test/template/values.schema.json
- test/template/README.md
- test/objects/README.md
- test/basic-types/README.md
- test/lists/README.md
- test/objects/values.schema.json
- test/lists/values.schema.json
- test/complex-types/README.md
- test/objects/values.yaml
- test/complex-types/values.schema.json
🧰 Additional context used
🪛 checkmake (0.2.2)
test/basic-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/complex-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/lists/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/objects/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
test/template/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 YAMLlint (1.37.1)
test/complex-types/values.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
test/lists/values.yaml
[error] 91-91: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
test/template/Makefile (1)
1-1: Makefile inclusion looks correct and matches the pattern used in other test dirs – LGTM
The file cleanly delegates to../test.mk, so all common targets remain centralized. No further action needed.
Summary by CodeRabbit
New Features
Documentation