feat(cf): Added helm chart - #750
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change imports the NVCT API Helm chart into the monorepo. It adds Kubernetes templates, Vault and remote configuration support, deployment and packaging commands, self-managed deployment documentation, and Cloud Tasks release validation. ChangesCloud Tasks Helm chart
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Operator
participant Makefile
participant Helm
participant KubernetesAPI
participant VaultAgentInjector
Operator->>Makefile: Run install
Makefile->>Helm: Install nvct-api chart
Helm->>KubernetesAPI: Apply rendered resources
KubernetesAPI->>VaultAgentInjector: Process Vault annotations
VaultAgentInjector->>KubernetesAPI: Inject Vault configuration
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/cloud-tasks/Makefile`:
- Around line 70-77: Update the Makefile targets package and push-oci so they no
longer invoke clean or delete packaged-charts automatically. Keep clean as a
separately invoked target, preserving artifact directories unless the user
explicitly runs cleanup.
- Around line 21-25: Define the advertised deploy and delete lifecycle targets
in the Makefile, reusing the existing install and uninstall behavior as
appropriate. Ensure the documented make deploy command succeeds and that delete
provides the corresponding teardown operation, while retaining the existing
target declarations and validation flow.
In `@deploy/helm/cloud-tasks/nvct-api/Chart.yaml`:
- Around line 1-7: Add chart-rendering coverage for the Helm chart, covering
default values plus both the remoteConfig and autoscaling conditional branches.
Use the repository-native test runner and verify the rendered manifests; if
testing is genuinely not applicable, document that justification in the Pull
Request instead.
In `@deploy/helm/cloud-tasks/nvct-api/templates/_helpers.tpl`:
- Around line 86-89: Update the comment above the Vault Agent Injector
annotations to reference nvcf-openbao-migrations migrations/18_setup_nvct.sh
instead of migrations/20_setup_nvct.sh, leaving the surrounding annotation
documentation unchanged.
In `@deploy/helm/cloud-tasks/nvct-api/templates/NOTES.txt`:
- Around line 3-4: Update the HTTP and gRPC endpoint hostnames in the chart
NOTES template to use the same nvct-api.fullname helper as the existing
endpoint, instead of hard-coding nvct-api, so release and name overrides produce
correct service addresses.
In `@deploy/helm/cloud-tasks/nvct-api/templates/serviceaccount.yaml`:
- Line 4: Use the existing nvct-api.fullname helper for all chart-owned resource
references: set metadata.name in
deploy/helm/cloud-tasks/nvct-api/templates/serviceaccount.yaml:4-4 and
deploy/helm/cloud-tasks/nvct-api/templates/service.yaml:4-4, and set
serviceAccountName in
deploy/helm/cloud-tasks/nvct-api/templates/deployment.yaml:30-30 to the same
helper.
In `@deploy/helm/cloud-tasks/nvct-api/values.yaml`:
- Around line 142-144: Remove the HOSTNAME entry from the env configuration in
values.yaml so Kubernetes can provide each pod’s unique hostname. Keep
AWS_REGION unchanged; use a separate service-specific variable only if a stable
logical name is required.
In `@deploy/helm/cloud-tasks/nvct-api/vault-agent-templates/secrets.json.tmpl`:
- Around line 5-6: Update the secrets template to serialize every secret value
with toJSON, including Cassandra credentials, JWT tokens, and image-pull-secret.
Replace each manually quoted interpolation with the toJSON result directly,
including the username and password fields shown in the Cassandra credentials
block.
In `@deploy/helm/cloud-tasks/README.md`:
- Around line 7-9: Add kubeconform to the prerequisites in the deployment README
and direct users to run make validate for manifest schema validation, reflecting
the Makefile target that renders and validates the Kubernetes manifests.
- Line 13: Update the README text around the ServiceAccount and JWT role
guidance to remove Markdown bold formatting and replace em dashes on the
referenced lines with ASCII punctuation; preserve the documented names,
namespace, and meaning while ensuring all committed text uses plain text and
ASCII characters.
In `@tools/ci/test-github-release.py`:
- Around line 539-568: Update both tests,
test_cf_initial_version_anchor_defaults_to_floor and
test_initial_version_anchor_honors_metadata, to seed a commit under
deploy/helm/cloud-tasks before calling synthesize_initial_version_anchor. Extend
or parameterize _make_service_repo so the existing deploy/helm/ess history
remains intact while the Cloud Tasks path has matching history for anchor
synthesis.
- Around line 552-588: Rename the three Cloud Tasks
tests—test_initial_version_anchor_honors_metadata,
test_initial_version_anchor_rejects_bad_semver, and
test_initial_version_anchor_rejects_empty_string—with a cloud_tasks prefix so
they do not overwrite ESS tests. In the metadata test, replace
_make_service_repo with _make_ct_service_repo, and make the same
repository-fixture change in the Cloud Tasks default-floor test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 47ba5662-2e03-4c9b-a9c1-8fd8eaada618
📒 Files selected for processing (21)
deploy/helm/cloud-tasks/.gitignoredeploy/helm/cloud-tasks/AGENTS.mddeploy/helm/cloud-tasks/Makefiledeploy/helm/cloud-tasks/README.mddeploy/helm/cloud-tasks/nvct-api/.helmignoredeploy/helm/cloud-tasks/nvct-api/Chart.yamldeploy/helm/cloud-tasks/nvct-api/templates/NOTES.txtdeploy/helm/cloud-tasks/nvct-api/templates/_helpers.tpldeploy/helm/cloud-tasks/nvct-api/templates/configmap-env.yamldeploy/helm/cloud-tasks/nvct-api/templates/configmap-remote-config.yamldeploy/helm/cloud-tasks/nvct-api/templates/configmap-vault-agent-template.yamldeploy/helm/cloud-tasks/nvct-api/templates/deployment.yamldeploy/helm/cloud-tasks/nvct-api/templates/hpa.yamldeploy/helm/cloud-tasks/nvct-api/templates/rbac-config-reader.yamldeploy/helm/cloud-tasks/nvct-api/templates/service.yamldeploy/helm/cloud-tasks/nvct-api/templates/serviceaccount.yamldeploy/helm/cloud-tasks/nvct-api/values.yamldeploy/helm/cloud-tasks/nvct-api/vault-agent-templates/secrets.json.tmpldeploy/helm/cloud-tasks/values.local.yamltools/ci/github-release-subprojects.jsontools/ci/test-github-release.py
| .PHONY: deploy delete status lint template clean package push-oci install uninstall validate | ||
|
|
||
| install: | ||
| ifndef values | ||
| $(error "values" variable is not set. Please specify with 'make deploy values=<path-to-your-values.yaml>') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the advertised deployment lifecycle targets.
Line 21 declares deploy and delete, but this Makefile only defines install and uninstall. The examples on Lines 8 and 25 direct users to make deploy, which fails.
Proposed fix
.PHONY: deploy delete status lint template clean package push-oci install uninstall validate
+deploy: install
+
+delete: uninstall
+
install:📝 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.
| .PHONY: deploy delete status lint template clean package push-oci install uninstall validate | |
| install: | |
| ifndef values | |
| $(error "values" variable is not set. Please specify with 'make deploy values=<path-to-your-values.yaml>') | |
| .PHONY: deploy delete status lint template clean package push-oci install uninstall validate | |
| deploy: install | |
| delete: uninstall | |
| install: | |
| ifndef values | |
| $(error "values" variable is not set. Please specify with 'make deploy values=<path-to-your-values.yaml>') |
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 22-22: Required target "all" is missing from the Makefile.
(minphony)
[warning] 22-22: Required target "test" is missing from the Makefile.
(minphony)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/cloud-tasks/Makefile` around lines 21 - 25, Define the advertised
deploy and delete lifecycle targets in the Makefile, reusing the existing
install and uninstall behavior as appropriate. Ensure the documented make deploy
command succeeds and that delete provides the corresponding teardown operation,
while retaining the existing target declarations and validation flow.
| clean: | ||
| rm -rf ./bin | ||
| rm -rf ./packaged-charts | ||
|
|
||
| package: clean lint | ||
| @echo "[package] Packaging Helm chart $(CHART_NAME) version $(CHART_VERSION)..." | ||
| @mkdir -p ./packaged-charts | ||
| @helm package $(helm_dir) -d ./packaged-charts/ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not delete artifacts during package or push.
make package invokes clean and deletes bin and packaged-charts. make push-oci deletes packaged-charts after a successful push. These normal workflow commands do not explicitly request cleanup.
Keep clean as a separately invoked target. Remove automatic deletion from package and push-oci.
Proposed fix
-package: clean lint
+package: lint
@@
- `@echo` "[push-oci] Cleaning up temporary package directory..."
- `@rm` -rf ./packaged-charts
- `@echo` "[push-oci] Cleanup complete."As per coding guidelines, never delete artifact directories without explicit user confirmation.
Also applies to: 89-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/cloud-tasks/Makefile` around lines 70 - 77, Update the Makefile
targets package and push-oci so they no longer invoke clean or delete
packaged-charts automatically. Keep clean as a separately invoked target,
preserving artifact directories unless the user explicitly runs cleanup.
Source: Coding guidelines
| apiVersion: v2 | ||
| name: helm-nvcf-nvct-api | ||
| description: NCP Compatible deployment of NVCT API (nvct-service) | ||
|
|
||
| type: application | ||
| version: 0.0.0 # autoversioning enabled via release pipeline | ||
| appVersion: "1.6.2" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add chart render coverage.
The supplied changes add values and conditional Helm behavior, but they include no chart rendering test. Add coverage for default rendering and the remoteConfig and autoscaling branches, or explain in the Pull Request why tests are not applicable.
As per coding guidelines, “Code changes must include tests, or the Pull Request must explain why tests are not applicable; run the repository-native test runner before committing.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/cloud-tasks/nvct-api/Chart.yaml` around lines 1 - 7, Add
chart-rendering coverage for the Helm chart, covering default values plus both
the remoteConfig and autoscaling conditional branches. Use the repository-native
test runner and verify the rendered manifests; if testing is genuinely not
applicable, document that justification in the Pull Request instead.
Source: Coding guidelines
| {{/* | ||
| Vault Agent Injector Annotations for JWT Auth | ||
| Bound OpenBao JWT role and KSA name: nvct-api (see nvcf-openbao-migrations migrations/20_setup_nvct.sh). | ||
| */}} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the OpenBao migration reference.
Line 88 refers to migrations/20_setup_nvct.sh, but the provisioned JWT role is owned by migrations/18_setup_nvct.sh. Update the comment.
Based on learnings, OpenBao paths and JWT roles for this service are provisioned in nvcf-openbao-migrations, specifically migrations/18_setup_nvct.sh.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/cloud-tasks/nvct-api/templates/_helpers.tpl` around lines 86 -
89, Update the comment above the Vault Agent Injector annotations to reference
nvcf-openbao-migrations migrations/18_setup_nvct.sh instead of
migrations/20_setup_nvct.sh, leaving the surrounding annotation documentation
unchanged.
Source: Learnings
| HTTP: http://nvct-api.{{ include "nvct-api.namespace" . }}.svc.cluster.local:8080 | ||
| gRPC: nvct-api.{{ include "nvct-api.namespace" . }}.svc.cluster.local:9090 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the chart service name.
Lines 3-4 hard-code nvct-api, while line 5 uses nvct-api.fullname. A release name, nameOverride, or fullnameOverride makes the HTTP and gRPC endpoints incorrect.
Proposed fix
-HTTP: http://nvct-api.{{ include "nvct-api.namespace" . }}.svc.cluster.local:8080
-gRPC: nvct-api.{{ include "nvct-api.namespace" . }}.svc.cluster.local:9090
+HTTP: http://{{ include "nvct-api.fullname" . }}.{{ include "nvct-api.namespace" . }}.svc.cluster.local:8080
+gRPC: {{ include "nvct-api.fullname" . }}.{{ include "nvct-api.namespace" . }}.svc.cluster.local:9090📝 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.
| HTTP: http://nvct-api.{{ include "nvct-api.namespace" . }}.svc.cluster.local:8080 | |
| gRPC: nvct-api.{{ include "nvct-api.namespace" . }}.svc.cluster.local:9090 | |
| HTTP: http://{{ include "nvct-api.fullname" . }}.{{ include "nvct-api.namespace" . }}.svc.cluster.local:8080 | |
| gRPC: {{ include "nvct-api.fullname" . }}.{{ include "nvct-api.namespace" . }}.svc.cluster.local:9090 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/cloud-tasks/nvct-api/templates/NOTES.txt` around lines 3 - 4,
Update the HTTP and gRPC endpoint hostnames in the chart NOTES template to use
the same nvct-api.fullname helper as the existing endpoint, instead of
hard-coding nvct-api, so release and name overrides produce correct service
addresses.
| "username": "{{ .Data.data.username }}", | ||
| "password": "{{ .Data.data.password }}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
printf '%s\n' '--- guidance ---'
find .. -name AGENTS.md -print
printf '%s\n' '--- target context ---'
sed -n '1,80p' deploy/helm/cloud-tasks/nvct-api/vault-agent-templates/secrets.json.tmpl
printf '%s\n' '--- related templates and encoder usage ---'
rg -n --glob '*.tmpl' --glob '*.yaml' --glob '*.yml' 'toJSON|toJson|jsonEncode|Data\.data|vault-agent-templates' .
printf '%s\n' '--- chart metadata and dependencies ---'
find deploy/helm/cloud-tasks/nvct-api -maxdepth 3 -type f -print | sortRepository: NVIDIA/nvcf
Length of output: 14763
🏁 Script executed:
printf '%s\n' '--- repository guidance ---'
cat AGENTS.md
cat deploy/helm/cloud-tasks/AGENTS.md
printf '%s\n' '--- chart and injector configuration ---'
cat deploy/helm/cloud-tasks/nvct-api/Chart.yaml
cat deploy/helm/cloud-tasks/nvct-api/templates/configmap-vault-agent-template.yaml
rg -n 'vault-agent|agent-inject|openbao|vault' deploy/helm/cloud-tasks deploy/stacks src/control-plane-services/cloud-tasks -g '!*.json.tmpl'
printf '%s\n' '--- comparable corrected templates ---'
cat deploy/helm/ess/ess-api/vault-agent-templates/secrets.json.tmpl
cat deploy/helm/api-keys-colocated/api-keys/vault-agent-templates/secrets.json.tmplRepository: NVIDIA/nvcf
Length of output: 41440
🌐 Web query:
Vault Agent template toJSON function support consul-template JSON escaping
💡 Result:
The standard toJSON and toJSONPretty functions in Vault Agent (which utilizes Consul Template) use the Go standard library's json.Marshal function [1][2]. By design, this function HTML-escapes characters such as <, >, and & in the output to prevent security issues when rendering JSON into HTML templates [1]. While this output remains valid JSON, it often causes issues when the JSON contains database connection strings, URLs, or other data where those specific characters are meaningful and should not be encoded (e.g., & in a query parameter becomes \u0026) [1][3]. To resolve this, you should use the following unescaped alternatives, which were specifically introduced to prevent this HTML-escaping behavior [4][3]: - toUnescapedJSON: Converts data to a JSON object without HTML escaping [3][5]. - toUnescapedJSONPretty: Converts data to a pretty-printed JSON object without HTML escaping [5]. These functions are available for use within your Vault Agent templates wherever standard JSON conversion is required [6][3]. For example: {{- with secret "database/creds/db-app" -}} {{.Data | toUnescapedJSON }} {{- end }} These functions are documented in the Consul Template templating language reference, which governs the syntax used by Vault Agent templates [3][7].
Citations:
- 1: toJSON and toJSONPretty messes up secret from Vault hashicorp/consul-template#1430
- 2: https://developer.hashicorp.com/vault/docs/agent-and-proxy/agent/template
- 3: https://developer.hashicorp.com/consul/docs/reference/consul-template/go
- 4: Allow unescaped json output hashicorp/consul-template#1432
- 5: https://github.com/hashicorp/consul-template/blob/master/docs/templating-language.md
- 6: How to format the output to json hashicorp/vault#18132
- 7: https://github.com/hashicorp/consul-template/blob/main/docs/templating-language.md
🏁 Script executed:
printf '%s\n' '--- relevant secret generation ---'
rg -n -C 4 'cassandra/creds|username|password|image-pull-secret|toJSON|toUnescapedJSON' \
migrations deploy/helm/cloud-tasks src/control-plane-services/cloud-tasks \
-g '*.sh' -g '*.yaml' -g '*.yml' -g '*.json' -g '*.tmpl' -g '*.md'
printf '%s\n' '--- all raw secret interpolations in the target template ---'
nl -ba deploy/helm/cloud-tasks/nvct-api/vault-agent-templates/secrets.json.tmpl
python3 - <<'PY'
import json
values = {
"safe": ("user", "password"),
"json_specials": ('u"ser\\name', "p\nass\tword"),
"html_specials": ("user&name", "p<ass>word"),
}
for name, (username, password) in values.items():
rendered = (
'{\n'
' "kv": {"cassandra": {\n'
f' "username": "{username}",\n'
f' "password": "{password}"\n'
' }}\n'
'}'
)
try:
json.loads(rendered)
result = "valid JSON"
except json.JSONDecodeError as exc:
result = f"invalid JSON: {exc.msg}"
print(f"{name}: {result}")
PYRepository: NVIDIA/nvcf
Length of output: 35926
🌐 Web query:
Consul Template toJSON toUnescapedJSON Vault Agent template functions
💡 Result:
In Consul Template and Vault Agent templates, toJSON and toUnescapedJSON are functions used to convert data (typically maps or results from tree or ls calls) into JSON strings, with a critical difference regarding HTML character handling [1][2][3]. toJSON The toJSON function serializes an object into a JSON string using the standard Go json.Marshal behavior [4]. This process automatically escapes certain characters, such as <, >, and &, as Unicode sequences (e.g., \u003c) to prevent potential security issues when the output is rendered in HTML contexts [4]. While this is generally safer, it can corrupt data like database connection strings or complex URIs that contain these characters, rendering them unusable [1][4]. toUnescapedJSON The toUnescapedJSON function performs the same serialization but explicitly disables HTML escaping [1][4]. It is designed for use cases where the exact characters must be preserved, such as when generating JSON containing database connection strings, URIs with query parameters, or other structured data where character modification would break the output [1][2]. Summary of Functions - toJSON: Serializes to JSON with default HTML escaping enabled [4]. - toUnescapedJSON: Serializes to JSON with HTML escaping disabled [1][5]. - toJSONPretty / toUnescapedJSONPretty: Both functions also have "Pretty" variants (e.g., toJSONPretty, toUnescapedJSONPretty) that add indentation to the output for better human readability [2][6]. Vault Agent templates use the same underlying Consul Template engine [3][7], meaning these functions are directly available for use when rendering Vault secrets into files [3]. You can apply them via pipes, for example: {{ tree "config" | explode | toUnescapedJSON }} [1][2].
Citations:
- 1: https://developer.hashicorp.com/consul/docs/reference/consul-template/go
- 2: https://github.com/hashicorp/consul-template/blob/main/docs/templating-language.md
- 3: https://developer.hashicorp.com/vault/docs/agent-and-proxy/agent/template
- 4: toJSON and toJSONPretty messes up secret from Vault hashicorp/consul-template#1430
- 5: Allow unescaped json output hashicorp/consul-template#1432
- 6: https://github.com/hashicorp/consul-template/blob/v0.41.4/docs/templating-language.md
- 7: https://www.vaultproject.io/docs/agent/template
🏁 Script executed:
printf '%s\n' '--- secret-writing helper ---'
rg -n -C 12 'write_secrets_kv|DEFAULT_CASSANDRA_PASSWORD|NVCF_API_SIDECARS_IMAGE_PULL_SECRET' migrations/openbao -g '*.sh' -g '*.yaml'
printf '%s\n' '--- target template with line numbers ---'
awk '{printf "%4d %s\n", NR, $0}' \
deploy/helm/cloud-tasks/nvct-api/vault-agent-templates/secrets.json.tmpl
printf '%s\n' '--- JSON encoding behavior for all raw string fields ---'
python3 - <<'PY'
import json
fields = {
"username": 'u"ser\\name\n',
"password": 'p"ass\\word\t',
"nvcf": 'jwt"token\\\n',
"notary": 'jwt"token\\\n',
"icms": 'jwt"token\\\n',
"ess": 'jwt"token\\\n',
"api-keys": 'jwt"token\\\n',
"reval": 'jwt"token\\\n',
"image-pull-secret": 'secret"\\\n',
}
raw = "\n".join(
f' "{json.dumps(key)[1:-1]}": "{value}"' for key, value in fields.items()
)
encoded = "\n".join(
f' {json.dumps(key)}: {json.dumps(value)}' for key, value in fields.items()
)
for label, document in (("raw interpolation", "{\n" + raw + "\n}"),
("JSON-encoded values", "{\n" + encoded + "\n}")):
try:
parsed = json.loads(document)
except json.JSONDecodeError as exc:
print(f"{label}: INVALID JSON ({exc.msg})")
else:
print(f"{label}: valid JSON; values preserved={parsed == fields}")
PYRepository: NVIDIA/nvcf
Length of output: 42922
🏁 Script executed:
python3 - <<'PY'
import json
fields = {
"username": 'u"ser\\name\n',
"password": 'p"ass\\word\t',
"nvcf": 'jwt"token\\\n',
"notary": 'jwt"token\\\n',
"icms": 'jwt"token\\\n',
"ess": 'jwt"token\\\n',
"api-keys": 'jwt"token\\\n',
"reval": 'jwt"token\\\n',
"image-pull-secret": 'secret"\\\n',
}
raw_members = [
f'{json.dumps(key)}: "{value}"'
for key, value in fields.items()
]
encoded_members = [
f'{json.dumps(key)}: {json.dumps(value)}'
for key, value in fields.items()
]
for label, members in (
("raw interpolation", raw_members),
("JSON-encoded values", encoded_members),
):
document = "{\n" + ",\n".join(members) + "\n}"
try:
parsed = json.loads(document)
except json.JSONDecodeError as exc:
print(f"{label}: INVALID JSON ({exc.msg})")
else:
print(f"{label}: valid JSON; values preserved={parsed == fields}")
PYRepository: NVIDIA/nvcf
Length of output: 262
Serialize every secret value with toJSON.
Raw interpolation can produce invalid JSON when a credential contains quotes, backslashes, or control characters. Apply toJSON to the Cassandra credentials, JWT tokens, and image-pull-secret, and remove the surrounding manual quotes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/cloud-tasks/nvct-api/vault-agent-templates/secrets.json.tmpl`
around lines 5 - 6, Update the secrets template to serialize every secret value
with toJSON, including Cassandra credentials, JWT tokens, and image-pull-secret.
Replace each manually quoted interpolation with the toJSON result directly,
including the username and password fields shown in the Cassandra credentials
block.
| - Kubernetes cluster with Helm installed | ||
| - Access to the container registry that hosts the nvct-service image | ||
| - OpenBao / Vault Agent Injector configured for JWT auth; service account and policies must match `nvcf-openbao-migrations` (`migrations/18_setup_nvct.sh`) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the validation command and dependency.
make lint does not run kubeconform. The Makefile defines make validate, which renders the manifest and validates it with kubeconform. Add kubeconform to prerequisites and direct users to make validate for schema validation.
Also applies to: 21-21
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/cloud-tasks/README.md` around lines 7 - 9, Add kubeconform to the
prerequisites in the deployment README and direct users to run make validate for
manifest schema validation, reflecting the Makefile target that renders and
validates the Kubernetes manifests.
|
|
||
| ## Deployment | ||
|
|
||
| The Kubernetes `ServiceAccount` name **must** be `nvct-api` and the pod namespace **must** match the JWT role bound in OpenBao (default from migrations: namespace `nvcf`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove disallowed documentation formatting.
Line 13 uses Markdown bold. Lines 31 and 33 use em dash characters. Rewrite these sentences with plain text and ASCII punctuation.
As per coding guidelines, "Documentation and committed text must avoid Markdown bold emphasis, emojis, em dashes, and non-ASCII characters."
Also applies to: 31-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/cloud-tasks/README.md` at line 13, Update the README text around
the ServiceAccount and JWT role guidance to remove Markdown bold formatting and
replace em dashes on the referenced lines with ASCII punctuation; preserve the
documented names, namespace, and meaning while ensuring all committed text uses
plain text and ASCII characters.
Source: Coding guidelines
| def test_cf_initial_version_anchor_defaults_to_floor(self): | ||
| with tempfile.TemporaryDirectory() as tmp: | ||
| root = Path(tmp) | ||
| self._make_service_repo(root) | ||
| service = { | ||
| "id": "cloud-tasks-helm", | ||
| "path": "deploy/helm/cloud-tasks", | ||
| "service_name": "helm-cloud-tasks-api", | ||
| } | ||
| with chdir(root), contextlib.redirect_stdout(io.StringIO()): | ||
| self.github_release.synthesize_initial_version_anchor(root, service) | ||
| self.assertIn("deploy/helm/cloud-tasks/v0.0.0", self._tags(root)) | ||
|
|
||
| def test_initial_version_anchor_honors_metadata(self): | ||
| metadata = json.loads( | ||
| Path(__file__).with_name("github-release-subprojects.json").read_text() | ||
| ) | ||
| service = self.github_release.find_service(metadata, "cloud-tasks-helm") | ||
| expected_tag = self.github_release.tag_for_version(service, service["initial_version"]) | ||
| default_floor_tag = self.github_release.tag_for_version( | ||
| service, self.github_release.INITIAL_RELEASE_FLOOR_VERSION | ||
| ) | ||
| with tempfile.TemporaryDirectory() as tmp: | ||
| root = Path(tmp) | ||
| self._make_service_repo(root) | ||
| with chdir(root), contextlib.redirect_stdout(io.StringIO()): | ||
| self.github_release.synthesize_initial_version_anchor(root, service) | ||
| tags = self._tags(root) | ||
| self.assertIn(expected_tag, tags) | ||
| self.assertNotIn(default_floor_tag, tags) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Seed the Cloud Tasks path before synthesizing its anchor.
Line 542 and Line 563 call _make_service_repo, which creates history only for deploy/helm/ess. The tested service path is deploy/helm/cloud-tasks. synthesize_initial_version_anchor finds no commit for that path, so the assertions on Lines 550 and 567 fail.
Proposed fix
- self._make_service_repo(root)
+ self._make_ct_service_repo(root)
@@
- self._make_service_repo(root)
+ self._make_ct_service_repo(root)📝 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.
| def test_cf_initial_version_anchor_defaults_to_floor(self): | |
| with tempfile.TemporaryDirectory() as tmp: | |
| root = Path(tmp) | |
| self._make_service_repo(root) | |
| service = { | |
| "id": "cloud-tasks-helm", | |
| "path": "deploy/helm/cloud-tasks", | |
| "service_name": "helm-cloud-tasks-api", | |
| } | |
| with chdir(root), contextlib.redirect_stdout(io.StringIO()): | |
| self.github_release.synthesize_initial_version_anchor(root, service) | |
| self.assertIn("deploy/helm/cloud-tasks/v0.0.0", self._tags(root)) | |
| def test_initial_version_anchor_honors_metadata(self): | |
| metadata = json.loads( | |
| Path(__file__).with_name("github-release-subprojects.json").read_text() | |
| ) | |
| service = self.github_release.find_service(metadata, "cloud-tasks-helm") | |
| expected_tag = self.github_release.tag_for_version(service, service["initial_version"]) | |
| default_floor_tag = self.github_release.tag_for_version( | |
| service, self.github_release.INITIAL_RELEASE_FLOOR_VERSION | |
| ) | |
| with tempfile.TemporaryDirectory() as tmp: | |
| root = Path(tmp) | |
| self._make_service_repo(root) | |
| with chdir(root), contextlib.redirect_stdout(io.StringIO()): | |
| self.github_release.synthesize_initial_version_anchor(root, service) | |
| tags = self._tags(root) | |
| self.assertIn(expected_tag, tags) | |
| self.assertNotIn(default_floor_tag, tags) | |
| def test_cf_initial_version_anchor_defaults_to_floor(self): | |
| with tempfile.TemporaryDirectory() as tmp: | |
| root = Path(tmp) | |
| self._make_ct_service_repo(root) | |
| service = { | |
| "id": "cloud-tasks-helm", | |
| "path": "deploy/helm/cloud-tasks", | |
| "service_name": "helm-cloud-tasks-api", | |
| } | |
| with chdir(root), contextlib.redirect_stdout(io.StringIO()): | |
| self.github_release.synthesize_initial_version_anchor(root, service) | |
| self.assertIn("deploy/helm/cloud-tasks/v0.0.0", self._tags(root)) | |
| def test_initial_version_anchor_honors_metadata(self): | |
| metadata = json.loads( | |
| Path(__file__).with_name("github-release-subprojects.json").read_text() | |
| ) | |
| service = self.github_release.find_service(metadata, "cloud-tasks-helm") | |
| expected_tag = self.github_release.tag_for_version(service, service["initial_version"]) | |
| default_floor_tag = self.github_release.tag_for_version( | |
| service, self.github_release.INITIAL_RELEASE_FLOOR_VERSION | |
| ) | |
| with tempfile.TemporaryDirectory() as tmp: | |
| root = Path(tmp) | |
| self._make_ct_service_repo(root) | |
| with chdir(root), contextlib.redirect_stdout(io.StringIO()): | |
| self.github_release.synthesize_initial_version_anchor(root, service) | |
| tags = self._tags(root) | |
| self.assertIn(expected_tag, tags) | |
| self.assertNotIn(default_floor_tag, tags) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/ci/test-github-release.py` around lines 539 - 568, Update both tests,
test_cf_initial_version_anchor_defaults_to_floor and
test_initial_version_anchor_honors_metadata, to seed a commit under
deploy/helm/cloud-tasks before calling synthesize_initial_version_anchor. Extend
or parameterize _make_service_repo so the existing deploy/helm/ess history
remains intact while the Cloud Tasks path has matching history for anchor
synthesis.
| def test_initial_version_anchor_honors_metadata(self): | ||
| metadata = json.loads( | ||
| Path(__file__).with_name("github-release-subprojects.json").read_text() | ||
| ) | ||
| service = self.github_release.find_service(metadata, "cloud-tasks-helm") | ||
| expected_tag = self.github_release.tag_for_version(service, service["initial_version"]) | ||
| default_floor_tag = self.github_release.tag_for_version( | ||
| service, self.github_release.INITIAL_RELEASE_FLOOR_VERSION | ||
| ) | ||
| with tempfile.TemporaryDirectory() as tmp: | ||
| root = Path(tmp) | ||
| self._make_service_repo(root) | ||
| with chdir(root), contextlib.redirect_stdout(io.StringIO()): | ||
| self.github_release.synthesize_initial_version_anchor(root, service) | ||
| tags = self._tags(root) | ||
| self.assertIn(expected_tag, tags) | ||
| self.assertNotIn(default_floor_tag, tags) | ||
|
|
||
| def test_initial_version_anchor_rejects_bad_semver(self): | ||
| service = { | ||
| "id": "cloud-tasks-helm", | ||
| "path": "deploy/helm/ess", | ||
| "service_name": "helm-cloud-tasks-api", | ||
| "initial_version": "not-a-version", | ||
| } | ||
| with self.assertRaises(SystemExit): | ||
| self.github_release.initial_floor_version(service) | ||
|
|
||
| def test_initial_version_anchor_rejects_empty_string(self): | ||
| service = { | ||
| "id": "cloud-tasks-helm", | ||
| "path": "deploy/helm/ess", | ||
| "service_name": "helm-cloud-tasks-api", | ||
| "initial_version": "", | ||
| } | ||
| with self.assertRaises(SystemExit): | ||
| self.github_release.initial_floor_version(service) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from collections import defaultdict
from pathlib import Path
path = Path("tools/ci/test-github-release.py")
tree = ast.parse(path.read_text())
for node in ast.walk(tree):
if isinstance(node, ast.ClassDef) and node.name == "GithubReleaseTest":
methods = defaultdict(list)
for item in node.body:
if isinstance(item, ast.FunctionDef):
methods[item.name].append(item.lineno)
for name, lines in sorted(methods.items()):
if len(lines) > 1:
print(f"{name}: {lines}")
PYRepository: NVIDIA/nvcf
Length of output: 325
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- duplicate methods and surrounding definitions ---'
nl -ba tools/ci/test-github-release.py | sed -n '430,610p'
printf '%s\n' '--- helper definitions and call sites ---'
rg -n -C 4 'def _make_(ct_service_repo|service_repo)|_make_(ct_service_repo|service_repo)\(' tools/ci/test-github-release.py
printf '%s\n' '--- metadata entry ---'
rg -n -C 8 '"cloud-tasks-helm"|"initial_version"' tools/ci/github-release-subprojects.json | head -80
printf '%s\n' '--- unittest-discovered method names ---'
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("tools/ci/test-github-release.py")
tree = ast.parse(path.read_text())
for cls in [n for n in ast.walk(tree) if isinstance(n, ast.ClassDef) and n.name == "GithubReleaseTest"]:
methods = {}
for item in cls.body:
if isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)) and item.name.startswith("test"):
methods[item.name] = item.lineno
for name, line in methods.items():
print(f"{line}: {name}")
PYRepository: NVIDIA/nvcf
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- duplicate methods and surrounding definitions ---'
sed -n '430,610p' tools/ci/test-github-release.py | cat -n
printf '%s\n' '--- helper definitions and call sites ---'
rg -n -C 4 'def _make_(ct_service_repo|service_repo)|_make_(ct_service_repo|service_repo)\(' tools/ci/test-github-release.py
printf '%s\n' '--- metadata entry ---'
rg -n -C 8 '"cloud-tasks-helm"|"initial_version"' tools/ci/github-release-subprojects.json | head -80
printf '%s\n' '--- unittest-discovered method names ---'
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("tools/ci/test-github-release.py")
tree = ast.parse(path.read_text())
for cls in ast.walk(tree):
if isinstance(cls, ast.ClassDef) and cls.name == "GithubReleaseTest":
methods = {}
for item in cls.body:
if isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)) and item.name.startswith("test"):
methods[item.name] = item.lineno
for name, line in methods.items():
print(f"{line}: {name}")
PYRepository: NVIDIA/nvcf
Length of output: 14691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- anchor implementation ---'
rg -n -C 12 'def (synthesize_initial_version_anchor|initial_floor_version|service_has_history|tag_for_version)' tools/ci
printf '%s\n' '--- all Cloud Tasks anchor tests ---'
rg -n -C 18 'cloud-tasks|test_cf_initial_version_anchor|test_initial_version_anchor_honors_metadata|test_initial_version_anchor_rejects' tools/ci/test-github-release.pyRepository: NVIDIA/nvcf
Length of output: 14541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
test_path = Path("tools/ci/test-github-release.py")
release_path = Path("tools/ci/github-release")
test_tree = ast.parse(test_path.read_text())
release_tree = ast.parse(release_path.read_text())
target_methods = {}
for node in ast.walk(test_tree):
if isinstance(node, ast.ClassDef) and node.name == "GithubReleaseTest":
for item in node.body:
if isinstance(item, ast.FunctionDef) and (
item.name.startswith("test_initial_version_anchor")
or item.name == "test_cf_initial_version_anchor_defaults_to_floor"
):
target_methods.setdefault(item.name, []).append(item)
print("test method definitions:")
for name, nodes in sorted(target_methods.items()):
calls = [
call.func.attr
for node in nodes
for call in ast.walk(node)
if isinstance(call, ast.Call)
and isinstance(call.func, ast.Attribute)
and isinstance(call.func.value, ast.Name)
and call.func.value.id == "self"
and call.func.attr.startswith("_make_")
]
print(f"{name}: definitions={len(nodes)}, helpers={calls}")
for node in ast.walk(release_tree):
if isinstance(node, ast.FunctionDef) and node.name == "synthesize_initial_version_anchor":
has_path_scoped_rev_list = any(
isinstance(call, ast.Call)
and isinstance(call.func, Name := ast.Name)
and Name.id == "run"
and len(call.args) >= 1
and isinstance(call.args[0], ast.List)
and any(
isinstance(element, ast.Constant) and element.value == "rev-list"
for element in call.args[0].elts
)
and any(
isinstance(element, ast.Subscript)
and isinstance(element.value, ast.Name)
and element.value.id == "service"
for element in call.args[0].elts
)
for call in ast.walk(node)
)
print(f"synthesize_initial_version_anchor path-scoped history lookup: {has_path_scoped_rev_list}")
break
PYRepository: NVIDIA/nvcf
Length of output: 797
Fix the Cloud Tasks anchor tests.
- Rename the Cloud Tasks methods at lines 552, 570, and 580 with a
cloud_tasksprefix. Their current names overwrite the ESS tests. - Use
_make_ct_service_repoin the Cloud Tasks default-floor and metadata tests._make_service_repocreates onlydeploy/helm/ess, so no Cloud Tasks history exists for the anchor lookup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/ci/test-github-release.py` around lines 552 - 588, Rename the three
Cloud Tasks tests—test_initial_version_anchor_honors_metadata,
test_initial_version_anchor_rejects_bad_semver, and
test_initial_version_anchor_rejects_empty_string—with a cloud_tasks prefix so
they do not overwrite ESS tests. In the metadata test, replace
_make_service_repo with _make_ct_service_repo, and make the same
repository-fixture change in the Cloud Tasks default-floor test.
Source: Coding guidelines
6c35e08 to
4e72436
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/control-plane-services/nats-auth-callout/go.mod`:
- Line 111: Make the gRPC dependency version consistent in go.mod: align the
google.golang.org/grpc requirement at v1.80.0 with the effective replacement, or
remove/update the replacement accordingly; if retaining the v1.79.3 pin, change
the requirement to v1.79.3 and document the reason.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 58ccc62c-d3c3-4054-bcd7-c0d7101905c3
📒 Files selected for processing (22)
deploy/helm/cloud-tasks/.gitignoredeploy/helm/cloud-tasks/AGENTS.mddeploy/helm/cloud-tasks/Makefiledeploy/helm/cloud-tasks/README.mddeploy/helm/cloud-tasks/nvct-api/.helmignoredeploy/helm/cloud-tasks/nvct-api/Chart.yamldeploy/helm/cloud-tasks/nvct-api/templates/NOTES.txtdeploy/helm/cloud-tasks/nvct-api/templates/_helpers.tpldeploy/helm/cloud-tasks/nvct-api/templates/configmap-env.yamldeploy/helm/cloud-tasks/nvct-api/templates/configmap-remote-config.yamldeploy/helm/cloud-tasks/nvct-api/templates/configmap-vault-agent-template.yamldeploy/helm/cloud-tasks/nvct-api/templates/deployment.yamldeploy/helm/cloud-tasks/nvct-api/templates/hpa.yamldeploy/helm/cloud-tasks/nvct-api/templates/rbac-config-reader.yamldeploy/helm/cloud-tasks/nvct-api/templates/service.yamldeploy/helm/cloud-tasks/nvct-api/templates/serviceaccount.yamldeploy/helm/cloud-tasks/nvct-api/values.yamldeploy/helm/cloud-tasks/nvct-api/vault-agent-templates/secrets.json.tmpldeploy/helm/cloud-tasks/values.local.yamlsrc/control-plane-services/nats-auth-callout/go.modtools/ci/github-release-subprojects.jsontools/ci/test-github-release.py
💤 Files with no reviewable changes (1)
- tools/ci/github-release-subprojects.json
🚧 Files skipped from review as they are similar to previous changes (11)
- deploy/helm/cloud-tasks/.gitignore
- deploy/helm/cloud-tasks/nvct-api/.helmignore
- deploy/helm/cloud-tasks/nvct-api/templates/NOTES.txt
- deploy/helm/cloud-tasks/nvct-api/Chart.yaml
- deploy/helm/cloud-tasks/values.local.yaml
- deploy/helm/cloud-tasks/AGENTS.md
- deploy/helm/cloud-tasks/README.md
- deploy/helm/cloud-tasks/nvct-api/vault-agent-templates/secrets.json.tmpl
- deploy/helm/cloud-tasks/nvct-api/values.yaml
- tools/ci/test-github-release.py
- deploy/helm/cloud-tasks/nvct-api/templates/_helpers.tpl
| google.golang.org/genproto/googleapis/api v0.0.0-20260401024825-9d38bb4040a9 // indirect | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20260401024825-9d38bb4040a9 // indirect | ||
| google.golang.org/grpc v1.79.3 // indirect | ||
| google.golang.org/grpc v1.80.0 // indirect |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cd src/control-plane-services/nats-auth-callout
effective_version="$(go list -m -f '{{.Version}}' google.golang.org/grpc)"
test "$effective_version" = "v1.80.0"Repository: NVIDIA/nvcf
Length of output: 149
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="src/control-plane-services/nats-auth-callout/go.mod"
sed -n '100,120p' "$file"
printf '\nMatching directives:\n'
rg -n 'google\.golang\.org/grpc|^replace' "$file"
printf '\nEffective module metadata:\n'
cd src/control-plane-services/nats-auth-callout
go list -m -f 'Path={{.Path}} Version={{.Version}} Replace={{if .Replace}}{{.Replace.Path}} {{.Replace.Version}}{{end}}' google.golang.org/grpcRepository: NVIDIA/nvcf
Length of output: 1128
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cd src/control-plane-services/nats-auth-callout
effective_version="$(
go list -m -f '{{if .Replace}}{{.Replace.Version}}{{else}}{{.Version}}{{end}}' google.golang.org/grpc
)"
printf 'effective replacement version: %s\n' "$effective_version"
test "$effective_version" = "v1.79.3"Repository: NVIDIA/nvcf
Length of output: 188
Make the gRPC version consistent.
The replace directive makes v1.79.3 effective instead of the required v1.80.0. Update or remove the replacement, or change the requirement to v1.79.3 and document the pin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/control-plane-services/nats-auth-callout/go.mod` at line 111, Make the
gRPC dependency version consistent in go.mod: align the google.golang.org/grpc
requirement at v1.80.0 with the effective replacement, or remove/update the
replacement accordingly; if retaining the v1.79.3 pin, change the requirement to
v1.79.3 and document the reason.
4e72436 to
63ae3bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ci/github-release-subprojects.json`:
- Around line 283-288: Add the required initial_version field to the
cloud-tasks-helm service entry in the release configuration, using the intended
initial semantic version expected by the release-anchor test. Preserve the
existing id, path, service_name, and legacy_tag_prefix values.
- Around line 283-288: Add the required initial_version metadata field to the
cloud-tasks-helm entry, alongside its existing service_name and
legacy_tag_prefix fields, using the expected initial release version format so
release tests can access service["initial_version"] before validating the tag.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 57740bc3-3d10-4fec-840e-900d6ddbe2da
📒 Files selected for processing (1)
tools/ci/github-release-subprojects.json
| { | ||
| "id": "cloud-tasks-helm", | ||
| "path": "deploy/helm/cloud-tasks", | ||
| "service_name": "helm-cloud-tasks-api", | ||
| "legacy_tag_prefix": "deploy/helm/cloud-tasks" | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add initial_version to the service entry.
The test at tools/ci/test-github-release.py, Line 607, reads service["initial_version"] for cloud-tasks-helm. This entry has no initial_version, so the test raises KeyError before it validates the release anchor. Add the intended initial semantic version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/ci/github-release-subprojects.json` around lines 283 - 288, Add the
required initial_version field to the cloud-tasks-helm service entry in the
release configuration, using the intended initial semantic version expected by
the release-anchor test. Preserve the existing id, path, service_name, and
legacy_tag_prefix values.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'def tag_for_version|legacy_tag_prefix|tag_format' tools/ci --glob '*.py' --glob '*.json'Repository: NVIDIA/nvcf
Length of output: 29840
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- release implementation symbols ---'
rg -n -C 12 'def (tag_for_version|initial_floor_version)|initial_version|legacy_tag_prefix|tag_format' tools/ci --glob '*.py' --glob '*.json'
printf '%s\n' '--- cloud-tasks metadata and test references ---'
rg -n -C 12 'cloud-tasks(-helm)?|initial_version' tools/ci --glob '*.py' --glob '*.json'Repository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd -t f 'github-release\.py$' tools/ci | head -n 1)
printf 'file=%s\n' "$file"
rg -n '^(def| def) (tag_for_version|initial_floor_version|synthesize_initial_version_anchor)|tag_format|legacy_tag_prefix|initial_version' "$file"
printf '%s\n' '--- implementation excerpts ---'
sed -n '120,220p' "$file"
sed -n '520,590p' "$file"Repository: NVIDIA/nvcf
Length of output: 10031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files 'tools/ci/*github-release*'
printf '%s\n' '--- implementation definitions ---'
rg -n -g '*.py' 'def tag_for_version|def initial_floor_version|def synthesize_initial_version_anchor|tag_format|legacy_tag_prefix' tools/ci --glob '!test-github-release.py'Repository: NVIDIA/nvcf
Length of output: 305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'def tag_for_version|def initial_floor_version|def synthesize_initial_version_anchor|tag_format|legacy_tag_prefix|initial_version' tools/ci/github-releaseRepository: NVIDIA/nvcf
Length of output: 7781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- focused test block ---'
sed -n '581,641p' tools/ci/test-github-release.py
printf '%s\n' '--- metadata generation references ---'
rg -n -C 8 'legacy_tag_prefix|initial_version|cloud-tasks-helm|cloud-tasks' tools/generate-subproject-ci tools/ci --glob '*.go' --glob '*.yaml' --glob '*.json' --glob '!test-github-release.py'
printf '%s\n' '--- repository references to candidate legacy tags ---'
rg -n 'deploy/helm/cloud-tasks(?:/v|v|[0-9])|cloud-tasks-helm' . --glob '!tools/ci/test-github-release.py' --glob '!tools/ci/github-release-subprojects.json'Repository: NVIDIA/nvcf
Length of output: 24396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
import json
from pathlib import Path
metadata_path = Path("tools/ci/github-release-subprojects.json")
implementation_path = Path("tools/ci/github-release")
tests_path = Path("tools/ci/test-github-release.py")
metadata = json.loads(metadata_path.read_text())
service = next(s for s in metadata["services"] if s["id"] == "cloud-tasks-helm")
version = "1.0.0"
default_tag = f'{service["path"]}/v{version}'
legacy_concatenation = f'{service["legacy_tag_prefix"]}{version}'
tree = ast.parse(implementation_path.read_text())
tag_for_version = next(
node for node in tree.body
if isinstance(node, ast.FunctionDef) and node.name == "tag_for_version"
)
tag_for_version_source = ast.get_source_segment(implementation_path.read_text(), tag_for_version)
test_source = tests_path.read_text()
required_metadata_lookup = 'service["initial_version"]' in test_source
print(json.dumps({
"service": service,
"initial_version_present": "initial_version" in service,
"default_release_tag_for_1.0.0": default_tag,
"legacy_prefix_direct_concatenation": legacy_concatenation,
"tag_for_version_implementation": tag_for_version_source,
"focused_test_requires_initial_version": required_metadata_lookup,
}, indent=2))
PYRepository: NVIDIA/nvcf
Length of output: 713
Add initial_version to cloud-tasks-helm. The release test reads service["initial_version"], but this metadata entry omits the key. The test raises KeyError before it checks the release tag.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/ci/github-release-subprojects.json` around lines 283 - 288, Add the
required initial_version metadata field to the cloud-tasks-helm entry, alongside
its existing service_name and legacy_tag_prefix fields, using the expected
initial release version format so release tests can access
service["initial_version"] before validating the tag.
63ae3bc to
67ec7e9
Compare
1 participant
Add support for Cloud-Tasks Helm charts
Closes #749
Summary by CodeRabbit
New Features
Documentation
Tests