-
Notifications
You must be signed in to change notification settings - Fork 119
[apps] Add enum of allowed values to resourcePreset
in all applications
#1117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ions It was present in some apps, such as managed kubernetes, but missing in others. bitnami/readme-generator removes enums after re-generating README, so now we patch them back using `yq` in Makefiles. Signed-off-by: Nick Volynkin <nick.volynkin@gmail.com>
WalkthroughExplicit enumerations for the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Makefile as Makefile
participant yq as yq
participant Schema as values.schema.json
Dev->>Makefile: make generate
Makefile->>yq: Run yq to set enum for resourcesPreset
yq->>Schema: Update resourcesPreset.enum with allowed values
Schema-->>Dev: Schema with enforced enum values
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (18)
packages/apps/vpn/Makefile (1)
5-5
: Consider centralizing the allowed-preset list to avoid duplication across appsEvery app now hard-codes the same
yq
expression with the literal array of presets.
If this list ever changes you’ll have to touch N Makefiles.
A small wrapper (e.g.$(PROJECT_ROOT)/scripts/patch-resource-enum.sh
) or a Make variable containing the JSON array would remove that duplication and lower maintenance cost.packages/apps/tcp-balancer/Makefile (2)
5-7
: Left-over clean-up line is now dead code
readme-generator
+yq -i
no longer createvalues.schema.json.tmp
, so therm -f values.schema.json.tmp
is redundant noise.- rm -f values.schema.json.tmp
Dropping it keeps the target minimal and prevents future confusion.
4-6
: Consider centralising the repeatedyq
patchEvery app Makefile now embeds the exact same two
yq
commands. A small helper (e.g.scripts/fix-schema.sh
) would reduce duplication and the risk of drift if the enum ever changes.No action required for this PR, but worth tracking.
packages/apps/nats/Makefile (1)
5-5
: Consider centralising the preset list to avoid N× duplicationThe hard-coded array is now repeated in every app Makefile. A single source of truth (e.g. a make variable in
scripts/package.mk
or an.include
-able preset file) would prevent future drift and simplify maintenance.packages/apps/nats/values.schema.json (1)
58-68
: Schema description can get out of sync with the enumThe
description
string manually lists the allowed values. Now that the canonical list lives in theenum
, consider trimming the description to something like
"Use a common resources preset when \
resources` is not set explicitly."or generating it from the enum during the same
yq` step to remove duplication.packages/apps/kafka/Makefile (1)
5-6
: Avoid hard-coding the same enum list twiceThe two
yq
calls duplicate the exact same array literal.
If the allowed presets ever change you’ll have to hunt down & update every copy across every app.A couple of lighter-weight options inside this Makefile:
PRESETS := ["none","nano","micro","small","medium","large","xlarge","2xlarge"] generate: readme-generator -v values.yaml -s values.schema.json -r README.md yq -i -o json --indent 4 '.properties.kafka.properties.resourcesPreset.enum = $(PRESETS)' values.schema.json yq -i -o json --indent 4 '.properties.zookeeper.properties.resourcesPreset.enum = $(PRESETS)' values.schema.jsonor even a single yq expression that patches both paths in one go.
Keeps things DRY and reduces future maintenance overhead.
packages/apps/ferretdb/Makefile (1)
5-5
: Flag external dependency & DRY opportunityThe hard-coded
yq
one-liner works, but it assumes:
yq
(mikefarah v4) is available in$PATH
on every dev & CI runner.- All apps keep their allowed presets list in sync by copy-pasting the same expression.
Consider:
- yq -i -o json --indent 4 '.properties.resourcesPreset.enum = ["none", "nano", "micro", "small", "medium", "large", "xlarge", "2xlarge"]' values.schema.json +# scripts/ensure-resources-preset-enum.sh values.schema.jsonand place the shared logic in a small script (or a
make
macro) so you:• declare the dependency once (e.g. via
tools.mk
orgo run github.com/mikefarah/yq/v4@latest
).
• avoid eight identical lines across Makefiles—future changes require editing only one place.Purely optional, but it keeps the repo DRY and reduces drift.
packages/apps/http-cache/Makefile (1)
26-27
: Factor-out the duplicated enum patch to keep maintenance trivialThe two
yq
invocations are identical except for the path prefix (haproxy
vsnginx
). Copy-pasting makes future edits error-prone (e.g. forgetting to change one list if the allowed presets ever evolve).A minimal refactor stays entirely inside the Makefile and avoids subshell loops:
- yq -i -o json --indent 4 '.properties.haproxy.properties.resourcesPreset.enum = ["none", "nano", "micro", "small", "medium", "large", "xlarge", "2xlarge"]' values.schema.json - yq -i -o json --indent 4 '.properties.nginx.properties.resourcesPreset.enum = ["none", "nano", "micro", "small", "medium", "large", "xlarge", "2xlarge"]' values.schema.json +# Patch both haproxy and nginx with the same enum +for path in .properties.haproxy.properties.resourcesPreset \ + .properties.nginx.properties.resourcesPreset ; do \ + yq -i -o json --indent 4 "$${path}.enum = [\"none\",\"nano\",\"micro\",\"small\",\"medium\",\"large\",\"xlarge\",\"2xlarge\"]" values.schema.json ; \ +doneThis keeps the single source of truth in one place and makes later bulk updates safer.
packages/apps/http-cache/values.schema.json (2)
35-37
: Consider removing hard-coded values from description to avoid future driftThe description still embeds the literal list
(allowed values: …)
. If the enum changes later, someone has to remember to update two places.You can safely drop the parenthetical note since JSON Schema tooling will already surface the allowed values, or generate it automatically from the enum if documentation is regenerated.
65-67
: Duplicate hard-coded values in the description here as well – see previous note about potential drift.packages/apps/redis/Makefile (1)
5-5
: Avoid copy-pasting the enum list across every app’s MakefileThe same hard-coded array now lives in many Makefiles. When the preset list changes you’ll have to touch each file plus all schemas. Consider:
• Moving the array to a central variable (e.g. in
scripts/package.mk
)
• Or generating the enum directly fromvalues.yaml
to keep a single source of truth.This keeps maintenance friction low and prevents drift.
packages/apps/redis/values.schema.json (1)
38-48
: Enum looks good; trim the duplicated list in the descriptionThe new
enum
cleanly enforces allowed values and already contains the default (“nano”). The description still lists the same values manually, which can get out of sync. You can either drop the parenthetical or generate it from the enum to avoid redundancy.packages/apps/clickhouse/Makefile (2)
8-8
: Consider centralising the enum list to avoid copy-paste driftEach app’s Makefile now embeds the same hard-coded list of presets.
When the list inevitably changes (e.g. a “4xlarge” tier is added) every Makefile will need editing.A small helper (e.g.
scripts/set-resource-preset-enum.sh
) or aPRESETS := none nano …
make variable reused across apps would keep the source of truth in one place and eliminate divergence.
8-8
: Fail fast ifyq
v4 is missingThe command relies on the mikefarah
yq
v4 CLI (-o
,--indent
, path expression syntax).
On systems where anotheryq
implementation is first in$PATH
, the target will silently do the wrong thing.Add an explicit version check to surface the problem early:
generate: readme-generator -v values.yaml -s values.schema.json -r README.md + @if ! yq --version 2>/dev/null | grep -q 'version 4'; then \ + echo "error: yq v4 is required" >&2; exit 1; \ + fi yq -i -o json --indent 4 '.properties.resourcesPreset.enum = ["none", "nano", "micro", "small", "medium", "large", "xlarge", "2xlarge"]' values.schema.jsonpackages/apps/postgres/Makefile (1)
5-5
: Avoid duplicating the enum list in every MakefileThe exact same
["none", "nano", "micro", "small", "medium", "large", "xlarge", "2xlarge"]
array is now hard-coded in each app’sgenerate
rule. When the list inevitably changes, every Makefile will need to be touched.Consider extracting the list to a shared variable/fragment (e.g.
scripts/resource-presets.mk
or an environment var) and re-using it:RESOURCE_PRESETS := ["none","nano","micro","small","medium","large","xlarge","2xlarge"] generate: readme-generator -v values.yaml -s values.schema.json -r README.md yq -i -o json --indent 4 \ '.properties.resourcesPreset.enum = '$(RESOURCE_PRESETS)'' \ values.schema.jsonThis keeps the truth in one place and prevents drift.
packages/apps/postgres/values.schema.json (1)
127-128
: Duplication between description and enumLine 127’s description still embeds the allowed values in plain text, while the formal list now lives in
enum
. Keeping both invites divergence the next time the list changes.
Recommend trimming the parentheses in the description or generating it from the same source as the enum.packages/apps/mysql/Makefile (1)
8-8
: Avoid hard-coding the enum list in every MakefileThe exact same array is duplicated across all application Makefiles. A central definition (e.g., in
scripts/common-envs.mk
or as a file undercharts/common/presets.json
) would:• guarantee consistency
• make future changes one-liner
• prevent subtle drifts between chartspackages/apps/mysql/values.schema.json (1)
75-88
: Keep description & enum in sync automaticallyThe description string manually enumerates the allowed values. When the enum list changes, the description can silently go stale. Consider generating the description from the enum (e.g., via a small JSON-aware script in the
generate
target) or shortening the text to “Allowed values are defined in the enum below.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/apps/clickhouse/Makefile
(1 hunks)packages/apps/clickhouse/values.schema.json
(1 hunks)packages/apps/ferretdb/Makefile
(1 hunks)packages/apps/ferretdb/values.schema.json
(1 hunks)packages/apps/http-cache/Makefile
(1 hunks)packages/apps/http-cache/values.schema.json
(3 hunks)packages/apps/kafka/Makefile
(1 hunks)packages/apps/kafka/values.schema.json
(3 hunks)packages/apps/mysql/Makefile
(1 hunks)packages/apps/mysql/values.schema.json
(1 hunks)packages/apps/nats/Makefile
(1 hunks)packages/apps/nats/values.schema.json
(1 hunks)packages/apps/postgres/Makefile
(1 hunks)packages/apps/postgres/values.schema.json
(1 hunks)packages/apps/rabbitmq/Makefile
(1 hunks)packages/apps/rabbitmq/values.schema.json
(1 hunks)packages/apps/redis/Makefile
(1 hunks)packages/apps/redis/values.schema.json
(1 hunks)packages/apps/tcp-balancer/Makefile
(1 hunks)packages/apps/tcp-balancer/values.schema.json
(1 hunks)packages/apps/vpn/Makefile
(1 hunks)packages/apps/vpn/values.schema.json
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (13)
packages/apps/vpn/values.schema.json (1)
33-46
: Enum addition correctly enforces validationThe enum aligns with the documented allowed values and includes the default
nano
, so schema validation will now catch typos. Looks good.packages/apps/tcp-balancer/values.schema.json (1)
66-79
: 👍 Enum introduced — validation gap closedAdding the explicit
enum
array forresourcesPreset
finally enforces the same contract that has been documented for a while. Default"nano"
is included, so no functional regression.
Just keep in mind that any existing releases that might have been using a typo or an ad-hoc preset string will now fail chart validation — worth highlighting in the release notes.packages/apps/kafka/values.schema.json (1)
37-46
: Enum addition looks correct
resourcesPreset
is now explicitly constrained, and the default"small"
sits inside the enum—valid JSON-Schema practice. 👍
No functional issues spotted here.Also applies to: 76-86
packages/apps/ferretdb/values.schema.json (1)
90-103
: Enum addition looks correct – verify generator doesn’t overwrite it
resourcesPreset
now has an explicit enum and the default"nano"
is included—good catch.
Because the enum is re-injected post-generation, make sure the README generator isn’t later run without the Makefile target (e.g. in a manual workflow) and wiping the enum again. A quick smoke test aftermake generate
on CI would guard against regressions.No further issues spotted.
packages/apps/rabbitmq/Makefile (1)
5-5
: Pinyq
version & avoid hard-coded list duplicationThe command relies on
yq
v4 syntax (-o json
,--indent
). CI images or developer machines running an earlier major version will silently fail.
Additionally, the exact same hard-coded enum is being duplicated across every app Makefile; drift is inevitable.+# ensure a known yq version (override allowed) +YQ ?= yq +YQ_MIN_VERSION := 4.44.1 # example +generate-yq-check: + @$(YQ) --version | grep -qE 'version ($(YQ_MIN_VERSION)|[5-9])' || \ + (echo "yq >= $(YQ_MIN_VERSION) required" && exit 1) + generate: readme-generator -v values.yaml -s values.schema.json -r README.md - yq -i -o json --indent 4 '.properties.resourcesPreset.enum = ["none", "nano", "micro", "small", "medium", "large", "xlarge", "2xlarge"]' values.schema.json + $(MAKE) generate-yq-check + $(YQ) -i -o json --indent 4 '.properties.resourcesPreset.enum = env(PRESETS)' values.schema.json + +# centralise the presets once +PRESETS := ["none","nano","micro","small","medium","large","xlarge","2xlarge"]Even just moving
PRESETS
to an upper-levelpackage.mk
would eliminate 11 near-identical lines.
[ suggest_optional_refactor ][ request_verification ]packages/apps/rabbitmq/values.schema.json (2)
37-48
: Remove duplicated “allowed values” from descriptionThe
enum
already makes the allowed values self-describing; listing them again in the description invites drift the next time the enum changes.- "description": "Use a common resources preset when `resources` is not set explicitly. (allowed values: none, nano, micro, small, medium, large, xlarge, 2xlarge)", + "description": "Use a common resources preset when `resources` is not set explicitly.",[ suggest_nitpick ]
39-48
: Enum addition looks correct
"nano"
remains the default and is included in the enum; ordering matches increasing sizes. Good constraint tightening.
[ approve_code_changes ]packages/apps/http-cache/values.schema.json (2)
36-46
: Schema enum added correctly – validation gap closedThe explicit
enum
guarantees Helm users get immediate feedback on invalidresourcesPreset
values.nano
remains the default and is inside the set, so the schema is internally consistent.No further action required.
66-76
: Mirrored enum looks good – same comments applyThe
nginx
section mirrors thehaproxy
change consistently.packages/apps/clickhouse/values.schema.json (1)
88-98
: Enum addition looks correct and default is consistent
"small"
remains the default and is included in the new enum list, ensuring validation passes out of the box.
No further issues spotted.packages/apps/postgres/Makefile (1)
5-5
: yq patch looks correct & idempotentThe command updates only the
enum
field without touching other attributes and remains safe to execute repeatedly. 👍packages/apps/postgres/values.schema.json (1)
128-138
: Schema change is accurateThe default
"micro"
value is part of the newly addedenum
, so validation will pass. The ordered list also matches presets used elsewhere.packages/apps/mysql/values.schema.json (1)
78-88
: Schema update looks correctThe enum accurately restricts
resourcesPreset
to the documented set and keeps the default"nano"
inside the allowed range. ✅
@@ -5,6 +5,7 @@ include ../../../scripts/package.mk | |||
|
|||
generate: | |||
readme-generator -v values.yaml -s values.schema.json -r README.md | |||
yq -i -o json --indent 4 '.properties.resourcesPreset.enum = ["none", "nano", "micro", "small", "medium", "large", "xlarge", "2xlarge"]' values.schema.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
yq
invocation may break on environments using v3 syntax
The flags -o json --indent 4
are valid for yq
v4 (mikefarah). In many CI / developer workstations the legacy v3 binary is still on the path, where the equivalent options are -j
and --indent=4
.
Consider adding an explicit version check (or pinning via the hack/tools
image) to avoid “unknown flag” failures during make generate
.
- yq -i -o json --indent 4 '.properties.resourcesPreset.enum = ["none", "nano", "micro", "small", "medium", "large", "xlarge", "2xlarge"]' values.schema.json
+# Require yq v4+
+ yq --version | grep -q 'version 4' || { echo "yq v4 is required"; exit 1; }
+ yq -i -o=json --indent=4 '.properties.resourcesPreset.enum = ["none","nano","micro","small","medium","large","xlarge","2xlarge"]' values.schema.json
📝 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.
yq -i -o json --indent 4 '.properties.resourcesPreset.enum = ["none", "nano", "micro", "small", "medium", "large", "xlarge", "2xlarge"]' values.schema.json | |
# Require yq v4+ | |
yq --version | grep -q 'version 4' || { echo "yq v4 is required"; exit 1; } | |
yq -i -o=json --indent=4 '.properties.resourcesPreset.enum = ["none","nano","micro","small","medium","large","xlarge","2xlarge"]' values.schema.json |
🤖 Prompt for AI Agents
In packages/apps/mysql/Makefile at line 8, the yq command uses flags compatible
only with yq v4, which may cause failures in environments with yq v3. To fix
this, add a version check before running yq to detect the installed version and
use the appropriate flags (-o json --indent 4 for v4, -j --indent=4 for v3).
Alternatively, pin the yq version by using a specific docker image or
installation method in the hack/tools setup to ensure consistent yq version
across environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a new yq on our runner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NickVolynkin, understood! With the new yq version on your runner, the version compatibility concern is resolved and the version check I suggested is unnecessary. The existing command should work correctly with yq v4+.
✏️ Learnings added
Learnt from: NickVolynkin
PR: cozystack/cozystack#1117
File: packages/apps/mysql/Makefile:8-8
Timestamp: 2025-06-26T04:29:24.806Z
Learning: The cozystack project uses yq v4+ on their CI runner, so yq v4 syntax (-o json --indent 4) is compatible and version checks are not needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merge after #1117 and #1155 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Improved clarity and detail in parameter descriptions across multiple app documentation files, especially for resource configuration options. * Expanded explanations for `resources` and `resourcesPreset` parameters, including explicit usage, allowed values, and fallback behavior. * Added new sections with YAML configuration examples and reference tables for resource presets in several app READMEs. * Corrected typos, improved formatting, and updated terminology for better readability and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
It was present in some apps, such as managed kubernetes, but was missing in others.
bitnami/readme-generator removes enums after re-generating README, so now we patch them back using
yq
in Makefiles.Summary by CodeRabbit