add redis support #130
add redis support #130
Conversation
WalkthroughAdds a new Redis Jsonnet module (config, statefulset, service, secret, configmap, serviceaccount, entrypoint), Tanka test environment and helper, updates Tanka dependencies/lockfile, and extends the GitHub Actions test workflow to include Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yaml (1)
39-41: Align kubeconform schema version with k8s-libsonnet 1.31Lockfile bumps k8s-libsonnet to 1.31; validating against Kubernetes 1.21 risks false errors/misses. Bump kubeconform’s --kubernetes-version to 1.31.0.
- ~/go/bin/kubeconform -kubernetes-version 1.21.0 -ignore-missing-schemas ${{ matrix.testcase }}.yaml + ~/go/bin/kubeconform -kubernetes-version 1.31.0 -ignore-missing-schemas ${{ matrix.testcase }}.yaml
🧹 Nitpick comments (9)
redis/config.libsonnet (2)
13-16: Use instance label derived from name for uniquenessConstant instance=redis can collide across multiple releases. Use s._name for uniqueness.
- commonLabels: { - 'app.kubernetes.io/instance': 'redis', - 'app.kubernetes.io/name': s.name, - }, + commonLabels: { + 'app.kubernetes.io/instance': s._name, + 'app.kubernetes.io/name': s.name, + },
17-21: Double-check default Redis image/tag6.2.8 is quite old; if users don’t override, they’ll run an EOL-ish version. Consider defaulting to a maintained 7.x tag or add a prominent comment encouraging override.
- image: { - registry: 'docker.io', - repository: 'library/redis', - tag: '6.2.8-bullseye', - }, + image: { + registry: 'docker.io', + repository: 'library/redis', + tag: '7.2.0', // TODO: keep in sync with supported major; override per env. + },redis/secret.libsonnet (2)
6-10: Prefer stringData over manual base64; avoid potential double-encoding and simplify authoringUse stringData and let Kubernetes handle base64. Also coerce to string to avoid type errors.
- secret: - secret.new(c._name, { - 'redis-password': std.base64(c.auth.password), - }) + - secret.metadata.withLabels(c.commonLabels), + secret: + secret.new(c._name, {}) + + { + stringData: { + 'redis-password': std.toString(c.auth.password), + }, + } + + secret.metadata.withLabels(c.commonLabels),
5-5: Guard against missing/empty password in configIf c.auth.password is absent/null, rendering will fail. Add a simple assertion/default upstream in config.libsonnet, or assert locally.
Would you like me to add an assert in config.libsonnet to enforce a non-empty string for c.auth.password?
tests/tanka/jsonnetfile.json (1)
27-35: Minor indentation inconsistency in the new dependency blockPurely cosmetic, but aligning indentation with adjacent entries improves readability.
tests/tanka/lib/letsbuilders/redis.libsonnet (1)
4-10: Allow password override via extVar for CI/local runsKeep 'password' as default for tests but enable override without code changes.
_config:: { auth: { - password: 'password', + // Override with: --ext-str REDIS_PASSWORD=... (falls back to 'password') + password: std.extVar('REDIS_PASSWORD') + '' || 'password', } },redis/redis.libsonnet (1)
1-3: Make params optional for a smoother DXDefault to {} so callers can do redis.redis() without boilerplate.
-local redis(params) = (import 'main.libsonnet') + { +local redis(params={}) = (import 'main.libsonnet') + { _config+: params, };redis/service.libsonnet (1)
27-34: Consider making the metrics port configurable.The metrics service hardcodes port 9121. Consider making this configurable through
c.metrics.portfor flexibility.- ports=servicePort.newNamed('http-metrics', 9121, 'metrics') + ports=servicePort.newNamed('http-metrics', c.metrics.port, 'metrics')redis/statefulset.libsonnet (1)
118-140: Consider adding StorageClass configuration support.The VolumeClaimTemplate doesn't specify a StorageClass. Consider making this configurable for environments that require specific storage classes.
Add support for StorageClass configuration in the PVC spec:
spec: { accessModes: [c.persistence.accessMode], resources: { requests: { storage: c.persistence.size, }, }, volumeMode: 'Filesystem', + [if std.objectHas(c.persistence, 'storageClass') && c.persistence.storageClass != '' then 'storageClassName']: c.persistence.storageClass, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.github/workflows/test.yaml(1 hunks)redis/config.libsonnet(1 hunks)redis/configmap.libsonnet(1 hunks)redis/main.libsonnet(1 hunks)redis/redis.libsonnet(1 hunks)redis/secret.libsonnet(1 hunks)redis/service.libsonnet(1 hunks)redis/statefulset.libsonnet(1 hunks)tests/tanka/environments/redis/main.jsonnet(1 hunks)tests/tanka/jsonnetfile.json(1 hunks)tests/tanka/jsonnetfile.lock.json(2 hunks)tests/tanka/lib/letsbuilders/redis.libsonnet(1 hunks)
🔇 Additional comments (15)
.github/workflows/test.yaml (1)
14-14: Matrix update LGTMAdding the redis testcase to CI is correct and minimal.
tests/tanka/jsonnetfile.lock.json (2)
36-41: Kubernetes lib bump: verify cross-lib compatibility and CI schemak8s-libsonnet moved to 1.31. Ensure all envs and CI validation use 1.31 schemas (see workflow suggestion) and that other libs (crossplane/ksonnet-util) don’t rely on removed API fields.
Would you like a quick grep script to scan for deprecated apiVersions across the repo?
57-65: Dependency switch to redis looks goodReplacing the local postgres dependency with redis aligns with the new test scenario and module wiring.
redis/config.libsonnet (2)
57-59: Good: enforce non-empty password at compile timeFail-fast on missing auth.password is the right default for safety.
82-89: PVC access mode key: confirm singular vs pluralYou use accessMode (singular). Ensure the statefulset template expands it to spec.accessModes: [ ... ]. If not, this will misrender the PVC.
I can draft the expansion logic if needed.
redis/main.libsonnet (1)
1-5: LGTM: composition order is saneconfig first, then resources; merge pattern is consistent with other libs here.
tests/tanka/jsonnetfile.json (1)
1-38: Dependency wiring and JSON validity confirmed
jsonnetfile.json is valid and all local dependency directories (../../kubernetes, ../../crossplane, ../../postgres, ../../redis) resolve correctly under tests/tanka.redis/redis.libsonnet (1)
5-7: LGTM: clean public entrypointExporting the function as a hidden field matches patterns in this repo.
redis/configmap.libsonnet (1)
1-10: LGTM! Clean and concise ConfigMap implementation.The implementation correctly uses the Kausal library to create a Kubernetes ConfigMap with proper naming conventions and label inheritance.
tests/tanka/environments/redis/main.jsonnet (2)
1-40: Well-structured test environment with comprehensive assertions.The test environment provides good coverage of the Redis deployment components including the StatefulSet, Services (master, headless, metrics), Secret, and ConfigMap.
33-33: Good practice: Verifying headless service configuration.The assertion correctly validates that the headless service has
clusterIP: 'None', which is essential for StatefulSet pod discovery.redis/service.libsonnet (1)
1-36: Comprehensive service definitions for Redis deployment.The module correctly defines three services:
- Master service for client connections
- Headless service for StatefulSet pod discovery
- Metrics service for Prometheus scraping
All services use appropriate selectors and port configurations.
redis/statefulset.libsonnet (3)
63-89: Remove the metrics exporter configuration concern.The exporter image string is correctly sourced from
c.metrics.image(i.e., docker.io/oliver006/redis_exporter:v1.76.0), not the Redis image.Likely an incorrect or invalid review comment.
100-101: Ensure nodeAffinity is initialized before useAccessing
c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTermswithout defaults can cause runtime errors if that path is unset—add a fallback or guard (e.g. default to an empty list) wherec.affinityis defined.
86-87: No change needed for REDIS_ADDR format
TheREDIS_ADDR=redis://localhost:6379value is already a valid Redis URI supported by oliver006/redis_exporter for TCP connections without no authentication.
| affinity: { | ||
| nodeAffinity: { | ||
| requiredDuringSchedulingIgnoredDuringExecution: { | ||
| nodeSelectorTerms: [], | ||
| }, | ||
| }, | ||
| }, | ||
| tolerations: [], |
There was a problem hiding this comment.
Default nodeAffinity makes pods unschedulable
requiredDuringSchedulingIgnoredDuringExecution with an empty nodeSelectorTerms array matches nothing. Defaults should not set “required” affinity. Replace with an empty affinity map so users can opt-in.
- affinity: {
- nodeAffinity: {
- requiredDuringSchedulingIgnoredDuringExecution: {
- nodeSelectorTerms: [],
- },
- },
- },
+ affinity: {},📝 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.
| affinity: { | |
| nodeAffinity: { | |
| requiredDuringSchedulingIgnoredDuringExecution: { | |
| nodeSelectorTerms: [], | |
| }, | |
| }, | |
| }, | |
| tolerations: [], | |
| affinity: {}, | |
| tolerations: [], |
🤖 Prompt for AI Agents
In redis/config.libsonnet around lines 37 to 44, the current default sets
nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution with an empty
nodeSelectorTerms which matches nothing and makes pods unschedulable; replace
that block with an empty affinity object (e.g., affinity: {}) so no required
affinity is imposed by default and users can opt-in to set nodeAffinity
themselves, leaving tolerations as-is (or explicitly keep tolerations: [] if you
want an empty default).
| configMap.new(c._name + '-configuration', c.config.content) + | ||
| configMap.metadata.withLabels(c.commonLabels), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that the config content is properly validated.
The ConfigMap creation relies on c.config.content being properly structured. Ensure that the config validation happens upstream to prevent runtime errors.
🏁 Script executed:
#!/bin/bash
# Description: Check the config.libsonnet implementation for content validation
# Find and examine the config.libsonnet file to understand config structure
fd -t f "config.libsonnet" --exec cat {}
# Check for any validation or default content structure
ast-grep --pattern 'config: {
$$$
content: $$$
$$$
}'Length of output: 4233
Implement content validation for c.config.content in config.libsonnet.
Ensure upstream that both master.conf and redis.conf keys exist and that their values are valid strings (e.g. by adding error guards when missing or of the wrong type) to prevent invalid ConfigMap creation.
🤖 Prompt for AI Agents
In redis/configmap.libsonnet around lines 7-8, c.config.content is passed
directly into configMap.new without validation; add guards to ensure
c.config.content is an object containing string keys "master.conf" and
"redis.conf". Modify the code to validate: check that c.config and
c.config.content exist, that both "master.conf" and "redis.conf" keys are
present, and that their values are strings; call error(...) with descriptive
messages when any check fails so invalid ConfigMaps can't be created, then pass
the validated content to configMap.new.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
redis/statefulset.libsonnet (1)
13-21: Security: stop passing password on the command line in probes.Reusing prior feedback: switch to REDISCLI_AUTH (or tcpSocket) to avoid exposing the password via args/process listings.
- container.livenessProbe.exec.withCommand(['/bin/sh', '-c', 'redis-cli -a $(REDIS_PASSWORD) ping']) + + container.livenessProbe.exec.withCommand(['/bin/sh', '-c', 'REDISCLI_AUTH="$REDIS_PASSWORD" redis-cli -h 127.0.0.1 -p 6379 ping']) + ... - container.readinessProbe.exec.withCommand(['/bin/sh', '-c', 'redis-cli -a $(REDIS_PASSWORD) ping']) + + container.readinessProbe.exec.withCommand(['/bin/sh', '-c', 'REDISCLI_AUTH="$REDIS_PASSWORD" redis-cli -h 127.0.0.1 -p 6379 ping']) +Alternative if auth isn’t required for health checks:
- container.livenessProbe.exec.withCommand([...]) + + container.livenessProbe.tcpSocket.withPort(6379) + ... - container.readinessProbe.exec.withCommand([...]) + + container.readinessProbe.tcpSocket.withPort(6379) +
🧹 Nitpick comments (5)
tests/tanka/lib/letsbuilders/redis.libsonnet (2)
4-7: Avoid committing plaintext passwords; prefer extVar for tests too.Use an external var to keep secrets out of the repo (the test harness can pass it via tk/jb).
_config:: { auth: { - password: 'password', + // pass with: tk eval --ext-code redis_password='"s3cr3t"' + password: std.extVar('redis_password'), } },
1-1: Reduce identifier shadowing for readability.Rename the local import to avoid confusion with the exported “redis” field.
-local redis = import 'redis/redis.libsonnet'; +local redisLib = import 'redis/redis.libsonnet'; ... - redis: redis.redis($._config) + redis: redisLib.redis($._config)Also applies to: 9-9
tests/tanka/environments/redis/main.jsonnet (1)
12-14: Make the service label dynamic.Use the namespace parameter instead of a hardcoded value to improve reuse.
labels: { - 'letsbuild.com/service': 'test', + 'letsbuild.com/service': namespace, },redis/statefulset.libsonnet (2)
100-101: Guard optional affinity config or ensure safe defaults.Directly indexing c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms will fail if any segment is unset. Confirm defaults to empty arrays/objects, or gate the setter.
Potential pattern if you want to guard here:
- statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms(c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms) + + (if std.objectHas(c, 'affinity') && + std.objectHas(c.affinity, 'nodeAffinity') && + std.objectHas(c.affinity.nodeAffinity, 'requiredDuringSchedulingIgnoredDuringExecution') && + std.objectHas(c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution, 'nodeSelectorTerms') + then statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms(c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms) + else self) +
26-26: Tighten pod/container security context (non-root, RO filesystem, no privilege escalation).Low-effort hardening; safe defaults for Redis and the exporter.
container.securityContext.withRunAsUser(1001) + + container.securityContext.withRunAsNonRoot(true) + + container.securityContext.withAllowPrivilegeEscalation(false) + + container.securityContext.withReadOnlyRootFilesystem(true) + ... container.securityContext.withRunAsUser(1001) + + container.securityContext.withRunAsNonRoot(true) + + container.securityContext.withAllowPrivilegeEscalation(false) + + container.securityContext.withReadOnlyRootFilesystem(true) + ... statefulSet.spec.template.spec.securityContext.withFsGroup(1001) + + statefulSet.spec.template.spec.securityContext.withFsGroupChangePolicy('OnRootMismatch') +Also applies to: 73-73, 107-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.github/workflows/test.yaml(1 hunks)redis/config.libsonnet(1 hunks)redis/configmap.libsonnet(1 hunks)redis/main.libsonnet(1 hunks)redis/redis.libsonnet(1 hunks)redis/secret.libsonnet(1 hunks)redis/service.libsonnet(1 hunks)redis/statefulset.libsonnet(1 hunks)tests/tanka/environments/redis/main.jsonnet(1 hunks)tests/tanka/jsonnetfile.json(1 hunks)tests/tanka/jsonnetfile.lock.json(2 hunks)tests/tanka/lib/letsbuilders/redis.libsonnet(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- redis/main.libsonnet
- redis/redis.libsonnet
- .github/workflows/test.yaml
- tests/tanka/jsonnetfile.json
- redis/service.libsonnet
- redis/config.libsonnet
- tests/tanka/jsonnetfile.lock.json
- redis/secret.libsonnet
- redis/configmap.libsonnet
🔇 Additional comments (1)
tests/tanka/environments/redis/main.jsonnet (1)
1-16: LGTM: environment scaffold and assertions are solid.Structure, imports, and assertions look consistent and useful.
Also applies to: 18-26, 27-40
PatTheSilent
force-pushed
the
redis
branch
2 times, most recently
from
d168348 to
ac6b467
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
redis/serviceaccount.libsonnet (3)
2-2: Avoid name shadowing between local alias and field nameUsing the same identifier
serviceAccountfor both the local alias and the exported field is easy to misread and can cause confusion in future edits.Apply:
-local serviceAccount = k.core.v1.serviceAccount; +local SA = k.core.v1.serviceAccount; @@ - serviceAccount: - serviceAccount.new(c._name) + - serviceAccount.metadata.withLabels(c.commonLabels), + serviceAccount: + SA.new(c._name) + + SA.metadata.withLabels(c.commonLabels),Also applies to: 7-9
7-9: Consider disabling token automount for least privilegeRedis typically doesn’t need K8s API access. Disabling automount reduces credential exposure. If a component needs the token, you can override at Pod level.
- serviceAccount.new(c._name) + - serviceAccount.metadata.withLabels(c.commonLabels), + serviceAccount.new(c._name) + + serviceAccount.metadata.withLabels(c.commonLabels) + + serviceAccount.withAutomountServiceAccountToken(false),
5-5: Tiny style nit: extra space around=Match common formatting.
- local c = $._config, + local c = $._config,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.github/workflows/test.yaml(1 hunks)redis/config.libsonnet(1 hunks)redis/configmap.libsonnet(1 hunks)redis/main.libsonnet(1 hunks)redis/redis.libsonnet(1 hunks)redis/secret.libsonnet(1 hunks)redis/service.libsonnet(1 hunks)redis/serviceaccount.libsonnet(1 hunks)redis/statefulset.libsonnet(1 hunks)tests/tanka/environments/redis/main.jsonnet(1 hunks)tests/tanka/jsonnetfile.json(1 hunks)tests/tanka/jsonnetfile.lock.json(2 hunks)tests/tanka/lib/letsbuilders/redis.libsonnet(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/tanka/jsonnetfile.json
- tests/tanka/lib/letsbuilders/redis.libsonnet
- .github/workflows/test.yaml
- redis/statefulset.libsonnet
- redis/secret.libsonnet
- tests/tanka/environments/redis/main.jsonnet
- tests/tanka/jsonnetfile.lock.json
- redis/main.libsonnet
- redis/redis.libsonnet
- redis/configmap.libsonnet
- redis/config.libsonnet
- redis/service.libsonnet
🔇 Additional comments (2)
redis/serviceaccount.libsonnet (2)
7-9: LGTM: minimal SA resource wired to config and labelsResource construction and label propagation via
$._configlook correct and align with the module layout described.
8-9: Ensure c._namespace and c.commonAnnotations exist in redis config
c._namespace and c.commonAnnotations aren’t defined in redis/config.libsonnet, so adding these metadata calls will break; add those fields to the config or remove the metadata.withAnnotations and withNamespace calls here.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
redis/config.libsonnet (1)
44-51: Fix default affinity: empty required nodeSelectorTerms makes pods unschedulable and the assert fails at evalDo not set requiredDuring... by default. Default to no affinity and let users opt-in.
- affinity: { - nodeAffinity: { - requiredDuringSchedulingIgnoredDuringExecution: { - nodeSelectorTerms: [], - assert std.length(self.nodeSelectorTerms) > 0 : 'nodeSelectorTerms cannot be empty', - }, - }, - }, + affinity: {},redis/statefulset.libsonnet (1)
13-21: Resolved: probes no longer expose password via argsSwitch to REDISCLI_AUTH is a safer approach than passing -a inline.
🧹 Nitpick comments (5)
redis/config.libsonnet (2)
20-23: Label semantics: use name='redis', instance= to avoid collisions across instancesFlip these to standard App/K8s label conventions.
- commonLabels: { - 'app.kubernetes.io/instance': 'redis', - 'app.kubernetes.io/name': s.name, - }, + commonLabels: { + 'app.kubernetes.io/instance': s._name, + 'app.kubernetes.io/name': 'redis', + },
80-86: Optional: set an eviction policy to avoid OOM under pressureConsider defaulting to an allkeys policy or at least documenting it.
'redis.conf': ||| # Enable AOF https://redis.io/topics/persistence#append-only-file appendonly yes # Disable RDB persistence, AOF persistence already enabled. save "" maxmemory %s + # Consider enabling an eviction policy suitable for your workload: + # maxmemory-policy allkeys-lru ||| % s.config._redisMaxMem,redis/statefulset.libsonnet (3)
63-74: Exporter lacks resource requests/limitsAdd small defaults to avoid eviction and improve scheduling.
_exporter_container:: container.new('metrics', '%(registry)s/%(repository)s:%(tag)s' % c.metrics.image) + + container.resources.withRequests({ cpu: '10m', memory: '32Mi' }) + + container.resources.withLimits({ cpu: '200m', memory: '128Mi' }) +
26-27: Harden container security (non-root, no exec escalation, drop caps, read-only fs)Apply to both containers.
- container.securityContext.withRunAsUser(1001) + + container.securityContext.withRunAsUser(1001) + + container.securityContext.withRunAsNonRoot(true) + + container.securityContext.withAllowPrivilegeEscalation(false) + + container.securityContext.withReadOnlyRootFilesystem(true) + + container.securityContext.capabilities.withDrop(['ALL']) +And similarly for the exporter container:
_exporter_container:: container.new('metrics', '%(registry)s/%(repository)s:%(tag)s' % c.metrics.image) + + container.securityContext.withRunAsNonRoot(true) + + container.securityContext.withAllowPrivilegeEscalation(false) + + container.securityContext.withReadOnlyRootFilesystem(true) + + container.securityContext.capabilities.withDrop(['ALL']) +
119-141: PVC annotations merge is good; consider exposing accessModes as a list laterCurrent single-mode is fine; just flagging future extensibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
redis/config.libsonnet(1 hunks)redis/statefulset.libsonnet(1 hunks)
🔇 Additional comments (2)
redis/config.libsonnet (2)
65-67: Good: hard fail on empty passwordPrevents accidental unauthenticated deployments.
24-28: Verify image/tag currency and CVEs6.2.8-bullseye is quite old; ensure it’s intentional and free of known vulns, or bump.
| mountPath: '/usr/local/etc/redis', | ||
| _redisMaxMem:: (function(x) std.format('%dmb', (std.parseInt(std.substr(x, 0, std.length(x) - 2) ) * 0.8 * (if std.endsWith(x, 'Gi') then 1024 else 1))))(s.resources.limits.memory), | ||
| content: { | ||
| 'master.conf': ||| | ||
| dir /data | ||
| # User-supplied master configuration: | ||
| databases 100 | ||
| rename-command FLUSHDB "" | ||
| rename-command FLUSHALL "" | ||
| # End of master configuration | ||
| |||, | ||
| 'redis.conf': ||| | ||
| # Enable AOF https://redis.io/topics/persistence#append-only-file | ||
| appendonly yes | ||
| # Disable RDB persistence, AOF persistence already enabled. | ||
| save "" | ||
| maxmemory %s | ||
| ||| % s.config._redisMaxMem, |
There was a problem hiding this comment.
Maxmemory calc can error (%d with float) and doesn’t normalize unit case
Floor the value and support Gi/Mi case-insensitively.
- _redisMaxMem:: (function(x) std.format('%dmb', (std.parseInt(std.substr(x, 0, std.length(x) - 2) ) * 0.8 * (if std.endsWith(x, 'Gi') then 1024 else 1))))(s.resources.limits.memory),
+ _redisMaxMem:: (function(raw)
+ local x = std.asciiLower(raw);
+ std.format(
+ '%dmb',
+ std.floor(
+ std.parseInt(std.substr(x, 0, std.length(x)-2)) *
+ (if std.endsWith(x, 'gi') then 1024
+ else if std.endsWith(x, 'mi') then 1
+ else error 'Unsupported memory unit for _config.resources.limits.memory: ' + raw) *
+ 0.8
+ )
+ )
+ )(s.resources.limits.memory),📝 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.
| mountPath: '/usr/local/etc/redis', | |
| _redisMaxMem:: (function(x) std.format('%dmb', (std.parseInt(std.substr(x, 0, std.length(x) - 2) ) * 0.8 * (if std.endsWith(x, 'Gi') then 1024 else 1))))(s.resources.limits.memory), | |
| content: { | |
| 'master.conf': ||| | |
| dir /data | |
| # User-supplied master configuration: | |
| databases 100 | |
| rename-command FLUSHDB "" | |
| rename-command FLUSHALL "" | |
| # End of master configuration | |
| |||, | |
| 'redis.conf': ||| | |
| # Enable AOF https://redis.io/topics/persistence#append-only-file | |
| appendonly yes | |
| # Disable RDB persistence, AOF persistence already enabled. | |
| save "" | |
| maxmemory %s | |
| ||| % s.config._redisMaxMem, | |
| mountPath: '/usr/local/etc/redis', | |
| _redisMaxMem:: (function(raw) | |
| local x = std.asciiLower(raw); | |
| std.format( | |
| '%dmb', | |
| std.floor( | |
| std.parseInt(std.substr(x, 0, std.length(x) - 2)) * | |
| (if std.endsWith(x, 'gi') then 1024 | |
| else if std.endsWith(x, 'mi') then 1 | |
| else error 'Unsupported memory unit for _config.resources.limits.memory: ' + raw) * | |
| 0.8 | |
| ) | |
| ) | |
| )(s.resources.limits.memory), | |
| content: { | |
| 'master.conf': ||| | |
| dir /data | |
| # User-supplied master configuration: | |
| databases 100 | |
| rename-command FLUSHDB "" | |
| rename-command FLUSHALL "" | |
| # End of master configuration | |
| |||, | |
| 'redis.conf': ||| | |
| # Enable AOF https://redis.io/topics/persistence#append-only-file | |
| appendonly yes | |
| # Disable RDB persistence, AOF persistence already enabled. | |
| save "" | |
| maxmemory %s | |
| ||| % s.config._redisMaxMem, |
🤖 Prompt for AI Agents
In redis/config.libsonnet around lines 69 to 86, the _redisMaxMem calculation
can produce a float formatted with %d and assumes a two-letter unit; change it
to normalize the unit case-insensitively (use std.lower on the input), detect
unit length (support Gi/G/Gi, Mi/M etc.), strip the correct suffix before
parsing, apply the appropriate multiplier (1024 for Gi, 1 for Mi or equivalent),
compute the 80% value, then floor the final numeric result (std.floor) before
formatting with '%dmb' so the formatter receives an integer and floats/doctypes
and mixed-case units are handled safely.
There was a problem hiding this comment.
The unit is cut off with std.substr so why would I care about normalizing it?
%d will print an unsigned integer. Why would it fail with a float? It'll just print the integer part of it, which I'm interested in. Completely pointless complexity.
There was a problem hiding this comment.
@PatTheSilent You're absolutely right on both points:
- The unit is indeed cut off with
std.substrbefore parsing, so case normalization for the numeric part isn't needed %dwill truncate floats to integers, which is exactly what you want here
The only potential case-sensitivity issue I was thinking of was the std.endsWith(x, 'Gi') check, but if you're standardizing on "Gi"/"Mi" format in your configs, that's not a concern either.
Thanks for the clarification - the original implementation is fine as-is.
(\ /)
( ._. )
o_("_")("_") *learning moment*
| statefulSet.spec.withServiceName(c._name + '-headless') + | ||
| statefulSet.spec.selector.withMatchLabels({ | ||
| 'app.kubernetes.io/component': 'master', | ||
| } + c.commonLabels) + | ||
| statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms(c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms) + | ||
| statefulSet.spec.template.spec.withTolerations(c.tolerations) + |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make affinity wiring robust to empty defaults
If config defaults to affinity: {}, current nested deref will break. Pass the entire affinity block instead.
- statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms(c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms) +
+ statefulSet.spec.template.spec.withAffinity(c.affinity) +📝 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.
| statefulSet.spec.withServiceName(c._name + '-headless') + | |
| statefulSet.spec.selector.withMatchLabels({ | |
| 'app.kubernetes.io/component': 'master', | |
| } + c.commonLabels) + | |
| statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms(c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms) + | |
| statefulSet.spec.template.spec.withTolerations(c.tolerations) + | |
| statefulSet.spec.withServiceName(c._name + '-headless') + | |
| statefulSet.spec.selector.withMatchLabels({ | |
| 'app.kubernetes.io/component': 'master', | |
| } + c.commonLabels) + | |
| statefulSet.spec.template.spec.withAffinity(c.affinity) + | |
| statefulSet.spec.template.spec.withTolerations(c.tolerations) + |
🤖 Prompt for AI Agents
In redis/statefulset.libsonnet around lines 96-101, the current chaining
accesses
c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms
which will throw when c.affinity is an empty object; instead pass the whole
affinity block to the template spec so defaults or empty objects are handled.
Replace the nested dereference with a single call that supplies the full
affinity (e.g. statefulSet.spec.template.spec.withAffinity(c.affinity || {})) so
the template receives the full affinity structure rather than drilling into
possibly-missing nested keys.
There was a problem hiding this comment.
withAffinity does not exist 🙄
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Let's examine the kausal library usage and see what affinity methods are available
rg -n "affinity" --type=libsonnet -A3 -B3
# Also check the config file to see how affinity is structured
cat redis/config.libsonnet | grep -A10 -B5 affinity
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Search for affinity usage in jsonnet files
rg -n "affinity" --type=json -A3 -B3
rg -n "affinity" -g "*.jsonnet" -A3 -B3
# Look for StatefulSet template spec methods
rg -n "template\.spec\." -g "*.libsonnet" -A2 -B2
# Check if there are other ways affinity is handled
rg -n "nodeAffinity" -g "*.libsonnet" -A3 -B3
Length of output: 13959
@PatTheSilent, you're absolutely right! Looking at the codebase patterns, the correct approach is to use conditional logic like the other files do. Here's the proper fix:
| statefulSet.spec.withServiceName(c._name + '-headless') + | |
| statefulSet.spec.selector.withMatchLabels({ | |
| 'app.kubernetes.io/component': 'master', | |
| } + c.commonLabels) + | |
| statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms(c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms) + | |
| statefulSet.spec.template.spec.withTolerations(c.tolerations) + | |
| statefulSet.spec.withServiceName(c._name + '-headless') + | |
| statefulSet.spec.selector.withMatchLabels({ | |
| 'app.kubernetes.io/component': 'master', | |
| } + c.commonLabels) + | |
| (if std.length(c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms) > 0 | |
| then statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms(c.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms) | |
| else {}) + | |
| statefulSet.spec.template.spec.withTolerations(c.tolerations) + |
This matches the pattern used in kubernetes/kubernetes.libsonnet and respects the assertion in your config that requires nodeSelectorTerms to be non-empty when used.
✏️ Learnings added
Learnt from: PatTheSilent
PR: letsbuilders/jsonnet-libs#130
File: redis/statefulset.libsonnet:96-101
Timestamp: 2025-09-04T10:49:00.253Z
Learning: In kausal StatefulSet templates, there is no `withAffinity()` method. Affinity should be set using conditional logic with the full nested path like `statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms()`, similar to the pattern used in kubernetes/kubernetes.libsonnet.
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.
There's no need for this. There's an assertion in config.libsonnet that checks if the array is empty
There was a problem hiding this comment.
🧩 Analysis chain
@PatTheSilent, you're absolutely right! I can see the assertion in the config that validates the nodeSelectorTerms array is non-empty. Since that validation happens at the config level, the conditional logic I suggested is indeed unnecessary. The original code is safe because it would fail early if the array is empty, preventing any runtime issues.
Thank you for the clarification!
🏁 Script executed:
#!/bin/bash
# Examine the config.libsonnet file for assertions related to nodeSelectorTerms
cat redis/config.libsonnet | grep -A10 -B5 assertion
# Also look for any validation of nodeSelectorTerms specifically
rg -n "nodeSelectorTerms" redis/config.libsonnet -A3 -B3
# Check for any error conditions related to empty arrays
rg -n "error\|assert" redis/config.libsonnet -A2 -B2
Length of output: 441
@PatTheSilent, you're absolutely correct! I can see the assertion on line 48 in redis/config.libsonnet:
assert std.length(self.nodeSelectorTerms) > 0 : 'nodeSelectorTerms cannot be empty'This assertion ensures that nodeSelectorTerms cannot be empty, which makes my suggested conditional logic completely unnecessary. The original code is safe because the validation happens at the config level, preventing any runtime issues with accessing the nested properties.
Thank you for pointing this out - the assertion-based validation is a much cleaner approach than conditional logic!
⛔ Learnings removed
Learnt from: PatTheSilent
PR: letsbuilders/jsonnet-libs#130
File: redis/statefulset.libsonnet:96-101
Timestamp: 2025-09-04T10:49:00.284Z
Learning: In kausal StatefulSet templates, there is no `withAffinity()` method. Affinity should be set using conditional logic with the full nested path like `statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms()`, similar to the pattern used in kubernetes/kubernetes.libsonnet.
Learnt from: PatTheSilent
PR: letsbuilders/jsonnet-libs#130
File: redis/statefulset.libsonnet:96-101
Timestamp: 2025-09-04T10:49:00.284Z
Learning: In kausal StatefulSet templates, there is no `withAffinity()` method. Affinity should be set using conditional logic with the full nested path like `statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms()`, similar to the pattern used in kubernetes/kubernetes.libsonnet.
🧠 Learnings used
Learnt from: PatTheSilent
PR: letsbuilders/jsonnet-libs#130
File: redis/statefulset.libsonnet:96-101
Timestamp: 2025-09-04T10:49:00.284Z
Learning: In kausal StatefulSet templates, there is no `withAffinity()` method. Affinity should be set using conditional logic with the full nested path like `statefulSet.spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.withNodeSelectorTerms()`, similar to the pattern used in kubernetes/kubernetes.libsonnet.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
redis/config.libsonnet (1)
44-51: Do not force required nodeAffinity by default; leave affinity empty so the lib works OOTBCurrent default asserts non-empty
nodeSelectorTerms, which breaks out-of-the-box usage and forces every consumer to patch scheduling. Make defaults zero-impact; let users opt-in via overlays.- affinity: { - nodeAffinity: { - requiredDuringSchedulingIgnoredDuringExecution: { - nodeSelectorTerms: [], - assert std.length(self.nodeSelectorTerms) > 0 : 'nodeSelectorTerms cannot be empty', - }, - }, - }, + # No required affinity by default; consumers can overlay via affinity+. + affinity: {},
🧹 Nitpick comments (4)
redis/config.libsonnet (2)
53-57: Hand PID 1 to Redis and fail fast: use exec and sh -ecThis ensures proper signal handling and exits on errors while retaining env expansion.
- command: [ - '/bin/sh', - '-c', - 'redis-server --include %(mountPath)s/redis.conf --include %(mountPath)s/master.conf --port ${REDIS_PORT} --requirepass ${REDIS_PASSWORD}' % { mountPath: s.config.mountPath }, - ], + command: [ + '/bin/sh', + '-ec', + 'exec redis-server --include "%(mountPath)s/redis.conf" --include "%(mountPath)s/master.conf" --port "${REDIS_PORT}" --requirepass "${REDIS_PASSWORD}"' % { mountPath: s.config.mountPath }, + ],
20-23: Set app.kubernetes.io/instance to the actual instance nameHardcoding
'redis'limits multi-instance usage and complicates selectors.- commonLabels: { - 'app.kubernetes.io/instance': 'redis', - 'app.kubernetes.io/name': s.name, - }, + commonLabels: { + 'app.kubernetes.io/instance': s._name, + 'app.kubernetes.io/name': s.name, + },tests/tanka/lib/letsbuilders/redis.libsonnet (1)
29-31: Avoid committing plaintext passwords even in testsOptional: fetch from an ext var in CI and keep a trivial local default in a separate overlay to prevent accidents.
.github/workflows/test.yaml (1)
21-24: Update actions to maintained majors
actions/checkout@v2andsetup-go@v2are deprecated; bump to current majors.- - uses: actions/checkout@v2 - - uses: actions/setup-go@v2 + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/test.yaml(2 hunks)redis/config.libsonnet(1 hunks)redis/serviceaccount.libsonnet(1 hunks)tests/tanka/lib/letsbuilders/redis.libsonnet(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- redis/serviceaccount.libsonnet
🔇 Additional comments (2)
tests/tanka/lib/letsbuilders/redis.libsonnet (1)
3-21: LGTM: sensible test overlayGood use of
affinity+andnodeSelectorTerms+to exercise scheduling overlays..github/workflows/test.yaml (1)
43-44: Verify kubeconform Kubernetes version support
Confirm that the CI-installedkubeconformrecognizes-kubernetes-version 1.32.0; if not, pin to a supported version or update the flag.
| go install github.com/jsonnet-bundler/jsonnet-bundler/cmd/jb@latest | ||
| go install github.com/grafana/tanka/cmd/tk@v0.20.0 | ||
| go install github.com/yannh/kubeconform/cmd/kubeconform@latest | ||
|
|
||
| echo "$HOME/.local/bin" >> "$GITHUB_PATH" | ||
|
|
There was a problem hiding this comment.
Binaries won’t be on PATH; go install puts them in GOPATH/bin, not ~/.local/bin
As written, jb, tk, and kubeconform may not be found. Either export GOBIN to $HOME/.local/bin during install or add $(go env GOPATH)/bin to PATH.
- run: |
- go install github.com/jsonnet-bundler/jsonnet-bundler/cmd/jb@latest
- go install github.com/grafana/tanka/cmd/tk@v0.20.0
- go install github.com/yannh/kubeconform/cmd/kubeconform@latest
-
- echo "$HOME/.local/bin" >> "$GITHUB_PATH"
+ mkdir -p "$HOME/.local/bin"
+ echo "$HOME/.local/bin" >> "$GITHUB_PATH"
+ GOBIN="$HOME/.local/bin" go install github.com/jsonnet-bundler/jsonnet-bundler/cmd/jb@latest
+ GOBIN="$HOME/.local/bin" go install github.com/grafana/tanka/cmd/tk@v0.20.0
+ GOBIN="$HOME/.local/bin" go install github.com/yannh/kubeconform/cmd/kubeconform@latest📝 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.
| go install github.com/jsonnet-bundler/jsonnet-bundler/cmd/jb@latest | |
| go install github.com/grafana/tanka/cmd/tk@v0.20.0 | |
| go install github.com/yannh/kubeconform/cmd/kubeconform@latest | |
| echo "$HOME/.local/bin" >> "$GITHUB_PATH" | |
| - run: | | |
| mkdir -p "$HOME/.local/bin" | |
| echo "$HOME/.local/bin" >> "$GITHUB_PATH" | |
| GOBIN="$HOME/.local/bin" go install github.com/jsonnet-bundler/jsonnet-bundler/cmd/jb@latest | |
| GOBIN="$HOME/.local/bin" go install github.com/grafana/tanka/cmd/tk@v0.20.0 | |
| GOBIN="$HOME/.local/bin" go install github.com/yannh/kubeconform/cmd/kubeconform@latest |
🤖 Prompt for AI Agents
.github/workflows/test.yaml around lines 26 to 31: the workflow installs Go
binaries but assumes they land in $HOME/.local/bin; by default go install writes
to $(go env GOPATH)/bin, so update the workflow to either set GOBIN to
$HOME/.local/bin before running go install or append $(go env GOPATH)/bin to
PATH after installs so jb, tk, and kubeconform are discoverable in subsequent
steps.
No description provided.