Skip to content

Conversation

@PatTheSilent
Copy link
Contributor

No description provided.

@coderabbitai
Copy link

coderabbitai bot commented Sep 26, 2025

Walkthrough

Refactors to read volumeClaimTemplate labels from ephemeral.metadata.labels and to source resources, accessModes, and storageClassName from ephemeral.spec.*; adds assertions checking a tmp-dir volumeMount and presence of a tmp-dir volume; introduces a deployment.volumes ephemeral tmp-dir with 20Gi in test config.

Changes

Cohort / File(s) Summary of changes
Kubernetes lib helper
kubernetes/k.libsonnet
Use ephemeral.metadata.labels for volumeClaimTemplate.metadata.labels; move resources, accessModes, and storageClassName to be read from ephemeral.spec.* with existing null-safety preserved; include metadata.labels in fromEphemeral composition.
Test — environment assertions
tests/tanka/environments/simple/main.jsonnet
Add assertions validating first container volumeMounts[0].name == 'tmp-dir', volumeMounts[0].mountPath == '/tmp', and that a volume named tmp-dir exists.
Test — deployment config volumes
tests/tanka/lib/letsbuilders/config.libsonnet
Add deployment.volumes with an ephemeral tmp-dir mounted at /tmp, applying podLabels and requesting 20Gi; minor formatting adjustments around podLabels in other test fixtures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive The pull request description is empty and does not provide any information about the changeset, so it is not possible to assess the intent or scope of the modifications. Please add a brief description summarizing the changes, such as the addition of ephemeral volume label support and related template updates, to give reviewers clear context.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the key change of adding support for ephemeral volume labels and directly reflects the updates to volumeClaimTemplate metadata fields, making it clear and specific for reviewers.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ephemeral-volume-labels

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c610cf and 7d8754a.

📒 Files selected for processing (3)
  • kubernetes/k.libsonnet (1 hunks)
  • tests/tanka/environments/simple/main.jsonnet (1 hunks)
  • tests/tanka/lib/letsbuilders/config.libsonnet (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/tanka/environments/simple/main.jsonnet
🔇 Additional comments (6)
kubernetes/k.libsonnet (1)

7-10: Guard ephemeral.metadata / ephemeral.spec before dereferencing

ephemeral defaults to {}, so accessing ephemeral.metadata or ephemeral.spec directly will throw when those keys are absent. This regresses the safety that was previously requested. Wrap each overlay in guards that first confirm the parent key exists before reading nested fields.

-          super.ephemeral.volumeClaimTemplate.metadata.withLabels(if std.objectHas(ephemeral.metadata, 'labels') then ephemeral.metadata.labels else {}) +
-          super.ephemeral.volumeClaimTemplate.spec.resources.withRequests(ephemeral.spec.resources.requests) +
-          super.ephemeral.volumeClaimTemplate.spec.withAccessModes(if std.objectHas(ephemeral.spec, 'accessModes') then ephemeral.accessModes else ['ReadWriteOnce']) +
-          super.ephemeral.volumeClaimTemplate.spec.withStorageClassName(if std.objectHas(ephemeral.spec, 'storageClassName') then ephemeral.storageClassName else 'gp3'),
+          (if std.objectHas(ephemeral, 'metadata') && std.objectHas(ephemeral.metadata, 'labels')
+           then super.ephemeral.volumeClaimTemplate.metadata.withLabels(ephemeral.metadata.labels)
+           else {}) +
+          (if std.objectHas(ephemeral, 'spec') && std.objectHas(ephemeral.spec, 'resources') &&
+              std.objectHas(ephemeral.spec.resources, 'requests')
+           then super.ephemeral.volumeClaimTemplate.spec.resources.withRequests(ephemeral.spec.resources.requests)
+           else {}) +
+          super.ephemeral.volumeClaimTemplate.spec.withAccessModes(
+            if std.objectHas(ephemeral, 'spec') && std.objectHas(ephemeral.spec, 'accessModes')
+            then ephemeral.spec.accessModes
+            else ['ReadWriteOnce']
+          ) +
+          super.ephemeral.volumeClaimTemplate.spec.withStorageClassName(
+            if std.objectHas(ephemeral, 'spec') && std.objectHas(ephemeral.spec, 'storageClassName')
+            then ephemeral.spec.storageClassName
+            else 'gp3'
+          ),
tests/tanka/lib/letsbuilders/config.libsonnet (5)

70-70: No functional change in statefulSet podLabels.

Skipping.


78-78: No functional change in statefulSet container block.

Skipping.


83-83: No functional change in job podLabels.

Skipping.


91-91: No functional change in job container block.

Skipping.


15-33: Make accessModes explicit on the ephemeral volume
Add under spec to avoid relying on defaults:

           spec: {
+            accessModes: ['ReadWriteOnce'],
             resources: {
               requests: {
                 storage: '20Gi',
               },
             },
           },

Ensure ephemeral.spec.* fields propagate to volumeClaimTemplate.spec.* as intended.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 (1)
tests/tanka/environments/simple/main.jsonnet (1)

33-33: Good assertion; consider strengthening it.

Optionally also assert the mountPath or make the check order‑independent to avoid brittleness if mount ordering changes.

Example additions:

+  assert std.equals(self.data.serviceDeployment.deployment.spec.template.spec.containers[0].volumeMounts[0].mountPath, '/tmp'),
+  assert std.member(
+    [m.name for m in self.data.serviceDeployment.deployment.spec.template.spec.containers[0].volumeMounts],
+    'tmp-dir'
+  ),
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1f8c16 and cde3df8.

📒 Files selected for processing (3)
  • kubernetes/k.libsonnet (1 hunks)
  • tests/tanka/environments/simple/main.jsonnet (1 hunks)
  • tests/tanka/lib/letsbuilders/config.libsonnet (2 hunks)
🔇 Additional comments (5)
tests/tanka/lib/letsbuilders/config.libsonnet (5)

15-32: Ephemeral volume config looks right.

  • Shape aligns with nested ephemeral.metadata/spec.
  • Labels sourced from podLabels are appropriate for the PVC template.
  • 20Gi request and path '/tmp' are clear.

If you need a non-default storage class, set ephemeral.spec.storageClassName here; the helper defaults to 'gp3'.


70-71: LGTM.

No functional change; trailing comma/style consistency is fine.


78-78: LGTM.

Trailing comma only.


83-84: LGTM.

Trailing comma only.


91-91: LGTM.

Trailing comma only.

@PatTheSilent PatTheSilent force-pushed the ephemeral-volume-labels branch from cde3df8 to 2c610cf Compare September 26, 2025 14:02
@PatTheSilent PatTheSilent force-pushed the ephemeral-volume-labels branch from 2c610cf to 7d8754a Compare September 26, 2025 14:06
@PatTheSilent PatTheSilent merged commit 9885135 into master Sep 26, 2025
4 checks passed
@PatTheSilent PatTheSilent deleted the ephemeral-volume-labels branch September 26, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants