feat: resolve pool computed fields at ConfigMap load time + export script#866
Merged
feat: resolve pool computed fields at ConfigMap load time + export script#866
Conversation
…ript ConfigMap pools previously required pre-expanded parsed_pod_template and parsed_resource_validations in every platform (verbose export). This added ~8K lines of bloat to the staging config (63% of the file). The ConfigMap loader now resolves these computed fields from template/ validation name references at load time, matching the same merge logic as Pool.calculate_pod_template() and Pool.calculate_resource_validations(). Changes: - configmap_loader: add _resolve_pool_computed_fields() with group template merge-by-key, default_mounts derivation, and platform field resolution - export script: strip parsed_* from platforms, not just pool level - Tests: 10 new test cases covering resolution, merge-by-key, default_mounts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds computed-field resolution during ConfigMap loading: merges referenced pod templates, resource validations, and group templates; computes platform labels, tolerations, and default_mounts; and adds unit/integration tests, an export CLI, and an upgrade guide for ConfigMap-mode migration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (2)
scripts/export_configs_to_helm.py (1)
100-105: Log the exception instead of silently passing.The bare
except Exception: passswallows all errors when reading the HTTP error body, making debugging harder.♻️ Proposed fix: log the exception
try: body = error.read().decode() print(f' {body[:200]}', file=sys.stderr) - except Exception: - pass + except Exception as read_error: + print(f' (Could not read error body: {read_error})', file=sys.stderr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/export_configs_to_helm.py` around lines 100 - 105, In the try/except around reading the HTTP error body (the block that calls error.read().decode() and prints body to sys.stderr), stop silently swallowing exceptions and log them; replace the bare "except Exception: pass" with "except Exception as e:" and call logging.exception or logging.error with the exception and context (e.g., logging.exception('Failed to read HTTP error body') or logging.error(..., exc_info=True)), ensuring the module imports and uses the standard logging logger so failures reading error.read() are recorded for debugging.src/service/core/config/configmap_loader.py (1)
290-296: Consider logging a warning for missing template references.When a template name in
common_pod_templatedoesn't exist inpod_templates, it's silently skipped (line 291). The downstreamPool.set_pod_templateinpostgres.pyraises an error for missing templates.While silent skipping may be intentional for flexibility, it could mask configuration errors. Consider adding a warning log to help users identify typos or missing template definitions.
💡 Optional: log warning for missing templates
for template_name in common_pod_template_names: if template_name in pod_templates: base_pod_template = recursive_dict_update( base_pod_template, copy.deepcopy(pod_templates[template_name]), merge_lists_on_name) + else: + logging.warning( + 'Pod template %r not found, skipping', template_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service/core/config/configmap_loader.py` around lines 290 - 296, The loop over common_pod_template_names silently ignores names not present in pod_templates; update the loop (which updates base_pod_template via recursive_dict_update and then assigns pool_data['parsed_pod_template']) to log a warning when template_name is missing from pod_templates so typos/missing definitions are visible; use the module logger (or existing logger used in this file) to emit a warning that includes the missing template_name and the current pool identifier/context (so downstream Pool.set_pod_template errors are easier to trace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/export_configs_to_helm.py`:
- Around line 251-252: The print statement in scripts/export_configs_to_helm.py
uses an unnecessary f-string (print(f'# Paste this into services.configs in your
Helm values.\n', file=sys.stderr)); update that call to use a regular string
literal by removing the leading "f" so it becomes a normal print of the static
message (ensure the same message and file=sys.stderr remain), locating the
change in the print call around the end of the file where this helper output is
emitted.
- Around line 76-83: PLATFORM_COMPUTED_FIELDS is missing derived fields that
_resolve_platform_fields() in configmap_loader computes; update the
PLATFORM_COMPUTED_FIELDS set in scripts/export_configs_to_helm.py to include
'tolerations', 'labels', and 'default_mounts' so these computed values are
stripped during export and don't persist stale values when
configmap_loader._resolve_platform_fields() later uses setdefault().
- Around line 284-306: The exported config keys in the configs dict use
camelCase (podTemplates, resourceValidations, groupTemplates, backendTests) but
configmap_loader.py's _EXPECTED_CONFIG_KEYS expects snake_case; update the
assignments in the export flow (where export_named_configs is called and results
assigned to configs) to use snake_case keys: pod_templates,
resource_validations, group_templates, backend_tests so the loader's unknown-key
check will recognize and load them correctly.
In `@src/service/core/config/configmap_loader.py`:
- Around line 379-386: Replace the conditional setdefault usage so tolerations,
labels, and default_mounts are always derived from the resolved template: assign
platform_data['tolerations'] = platform_pod_template.get('spec',
{}).get('tolerations', []), platform_data['labels'] =
platform_pod_template.get('spec', {}).get('nodeSelector', {}), and
platform_data['default_mounts'] = _get_default_mounts(platform_pod_template);
update the code around platform_pod_template and platform_data to perform these
unconditional assignments (referencing platform_pod_template, platform_data, and
_get_default_mounts).
---
Nitpick comments:
In `@scripts/export_configs_to_helm.py`:
- Around line 100-105: In the try/except around reading the HTTP error body (the
block that calls error.read().decode() and prints body to sys.stderr), stop
silently swallowing exceptions and log them; replace the bare "except Exception:
pass" with "except Exception as e:" and call logging.exception or logging.error
with the exception and context (e.g., logging.exception('Failed to read HTTP
error body') or logging.error(..., exc_info=True)), ensuring the module imports
and uses the standard logging logger so failures reading error.read() are
recorded for debugging.
In `@src/service/core/config/configmap_loader.py`:
- Around line 290-296: The loop over common_pod_template_names silently ignores
names not present in pod_templates; update the loop (which updates
base_pod_template via recursive_dict_update and then assigns
pool_data['parsed_pod_template']) to log a warning when template_name is missing
from pod_templates so typos/missing definitions are visible; use the module
logger (or existing logger used in this file) to emit a warning that includes
the missing template_name and the current pool identifier/context (so downstream
Pool.set_pod_template errors are easier to trace).
🪄 Autofix (Beta)
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: Pro Plus
Run ID: 60c443fe-75a3-4faf-956b-c51c06669386
📒 Files selected for processing (4)
scripts/export_configs_to_helm.pysrc/service/core/config/configmap_loader.pysrc/service/core/config/tests/test_configmap_loader_integration.pysrc/service/core/config/tests/test_configmap_loader_unit.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
==========================================
+ Coverage 44.23% 44.61% +0.37%
==========================================
Files 207 207
Lines 27641 27729 +88
Branches 7906 7953 +47
==========================================
+ Hits 12227 12370 +143
+ Misses 15298 15245 -53
+ Partials 116 114 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Move export_configs_to_helm.py from scripts/ to deployments/upgrades/ alongside the existing upgrade docs. Add 6_2_to_6_3_upgrade.md covering: - Exporting existing configs with the migration script - Enabling ConfigMap mode in Helm values - Creating K8s Secrets for credentials - Reverting to DB mode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
deployments/upgrades/export_configs_to_helm.py (2)
76-83:⚠️ Potential issue | 🟠 MajorStrip every platform field that the loader derives from template references.
src/service/core/config/configmap_loader.py:357-396recomputestolerations,labels, anddefault_mountsfor each platform from the resolved pod template, but this exporter currently leaves those values in place. Because the loader usessetdefault()for them, stale exported values will survive a round-trip instead of being refreshed from the source template.Suggested fix
PLATFORM_COMPUTED_FIELDS = { 'parsed_resource_validations', 'parsed_pod_template', 'parsed_group_templates', + 'tolerations', + 'labels', + 'default_mounts', }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/upgrades/export_configs_to_helm.py` around lines 76 - 83, PLATFORM_COMPUTED_FIELDS currently omits some fields that the config loader recomputes (tolerations, labels, default_mounts), causing stale values to persist; update the PLATFORM_COMPUTED_FIELDS set in export_configs_to_helm.py to include 'tolerations', 'labels', and 'default_mounts' so the exporter strips these platform fields before exporting, matching the loader behavior in src/service/core/config/configmap_loader.py (the loader logic around parsed_resource_validations/parsed_pod_template references these derived fields).
284-306:⚠️ Potential issue | 🔴 CriticalExport named config sections under the loader’s snake_case keys.
The ConfigMap loader resolves pool references from
managed_configs['pod_templates'],managed_configs['resource_validations'], andmanaged_configs['group_templates']insrc/service/core/config/configmap_loader.py:252-340. Emitting camelCase keys here breaks that round-trip, so exported pools/platforms cannot resolve their referenced templates or validations when reloaded from YAML. Use the loader’s snake_case keys consistently here as well.Suggested fix
if pod_templates: - configs['podTemplates'] = pod_templates + configs['pod_templates'] = pod_templates @@ if resource_validations: - configs['resourceValidations'] = resource_validations + configs['resource_validations'] = resource_validations @@ if group_templates: - configs['groupTemplates'] = group_templates + configs['group_templates'] = group_templates @@ if backend_tests: - configs['backendTests'] = backend_tests + configs['backend_tests'] = backend_tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/upgrades/export_configs_to_helm.py` around lines 284 - 306, The exported config keys are using camelCase ('podTemplates', 'resourceValidations', 'groupTemplates', 'backendTests') which breaks the ConfigMap loader that expects snake_case keys like managed_configs['pod_templates'], ['resource_validations'], and ['group_templates']; update the assignments that set entries on the configs dict (the blocks that call export_named_configs and then do configs['...'] = ...) to use the loader's snake_case keys ('pod_templates', 'resource_validations', 'group_templates', and 'backend_tests') so exported YAML round-trips correctly via export_named_configs and the configmap loader.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/upgrades/export_configs_to_helm.py`:
- Around line 234-237: The --include-defaults flag is parsed but never used;
update the logic that builds the configs list (the code that populates configs)
to conditionally include chart-default-matching entries only when
args.include_defaults is True: add a predicate (e.g., is_default_config or reuse
existing detection logic) and filter configs = [c for c in all_configs if
args.include_defaults or not is_default_config(c)], ensuring references to
parser.add_argument and args.include_defaults are used and that the default
behavior remains to exclude defaults.
- Around line 91-96: The fetch function constructs url = f'{base_url}{path}' and
calls urllib.request.urlopen without validating the URL scheme; add explicit
validation in fetch to parse the constructed URL (using urllib.parse.urlparse)
and ensure url.scheme is 'http' or 'https' before calling
urllib.request.urlopen, raising a clear exception (e.g., ValueError) or
returning an error if the scheme is invalid; reference the fetch function and
the base_url/url variables when making this change so the check runs immediately
after building url and prior to the urlopen call.
---
Duplicate comments:
In `@deployments/upgrades/export_configs_to_helm.py`:
- Around line 76-83: PLATFORM_COMPUTED_FIELDS currently omits some fields that
the config loader recomputes (tolerations, labels, default_mounts), causing
stale values to persist; update the PLATFORM_COMPUTED_FIELDS set in
export_configs_to_helm.py to include 'tolerations', 'labels', and
'default_mounts' so the exporter strips these platform fields before exporting,
matching the loader behavior in src/service/core/config/configmap_loader.py (the
loader logic around parsed_resource_validations/parsed_pod_template references
these derived fields).
- Around line 284-306: The exported config keys are using camelCase
('podTemplates', 'resourceValidations', 'groupTemplates', 'backendTests') which
breaks the ConfigMap loader that expects snake_case keys like
managed_configs['pod_templates'], ['resource_validations'], and
['group_templates']; update the assignments that set entries on the configs dict
(the blocks that call export_named_configs and then do configs['...'] = ...) to
use the loader's snake_case keys ('pod_templates', 'resource_validations',
'group_templates', and 'backend_tests') so exported YAML round-trips correctly
via export_named_configs and the configmap loader.
🪄 Autofix (Beta)
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: Pro Plus
Run ID: 84769855-af2e-4d58-b1c4-3ff025f1e438
📒 Files selected for processing (2)
deployments/upgrades/6_2_to_6_3_upgrade.mddeployments/upgrades/export_configs_to_helm.py
✅ Files skipped from review due to trivial changes (1)
- deployments/upgrades/6_2_to_6_3_upgrade.md
Fixes from CodeRabbit review on #866: - configmap_loader: change setdefault → unconditional assignment for tolerations, labels, default_mounts so stale values from YAML are always overwritten by resolution from the pod template - configmap_loader: log warnings when referenced pod templates, resource validations, or group templates are not found (catches typos) - export script: add tolerations, labels, default_mounts to PLATFORM_COMPUTED_FIELDS so they're stripped from exports - export script: log error instead of bare except on HTTP error body Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
deployments/upgrades/export_configs_to_helm.py (3)
288-310:⚠️ Potential issue | 🔴 CriticalEmit snake_case section names so the loader will accept them.
src/service/core/config/configmap_loader.pyonly recognizespod_templates,resource_validations,group_templates, andbackend_tests. Exporting the camelCase variants here means those sections are treated as unknown keys and dropped on import.🐛 Proposed fix
print('Exporting pod templates...', file=sys.stderr) pod_templates = export_named_configs( base_url, headers, 'pod_template') if pod_templates: - configs['podTemplates'] = pod_templates + configs['pod_templates'] = pod_templates @@ print('Exporting resource validations...', file=sys.stderr) resource_validations = export_named_configs( base_url, headers, 'resource_validation') if resource_validations: - configs['resourceValidations'] = resource_validations + configs['resource_validations'] = resource_validations @@ print('Exporting group templates...', file=sys.stderr) group_templates = export_named_configs( base_url, headers, 'group_template') if group_templates: - configs['groupTemplates'] = group_templates + configs['group_templates'] = group_templates @@ print('Exporting backend tests...', file=sys.stderr) backend_tests = export_named_configs( base_url, headers, 'backend_test') if backend_tests: - configs['backendTests'] = backend_tests + configs['backend_tests'] = backend_tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/upgrades/export_configs_to_helm.py` around lines 288 - 310, The exported config section keys are using camelCase so the loader (configmap_loader.py) drops them; update the assignments after calling export_named_configs (for 'pod_template', 'resource_validation', 'group_template', 'backend_test') to set snake_case keys on the configs dict (use configs['pod_templates'], configs['resource_validations'], configs['group_templates'], configs['backend_tests'] respectively) while keeping the export_named_configs calls and variables (pod_templates, resource_validations, group_templates, backend_tests) unchanged.
94-99:⚠️ Potential issue | 🟠 MajorReject non-HTTP(S) URLs before calling
urlopen().Line 96 builds directly from user input and Line 99 opens it without a scheme check. That still allows
file:and other handlers, which is risky for a migration CLI.🐛 Proposed fix
import argparse import json import os import sys import urllib.request import urllib.error +import urllib.parse @@ def fetch(base_url, path, headers): """Fetch JSON from the OSMO API.""" url = f'{base_url}{path}' + parsed_url = urllib.parse.urlsplit(url) + if parsed_url.scheme not in {'http', 'https'} or not parsed_url.netloc: + raise ValueError(f'Unsupported URL for export: {url!r}') req = urllib.request.Request(url, headers=headers) try:#!/bin/bash # Verify that fetch() still calls urlopen() without an explicit scheme guard. sed -n '94,111p;243,247p' deployments/upgrades/export_configs_to_helm.py rg -n "urlsplit|urlparse|scheme" deployments/upgrades/export_configs_to_helm.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/upgrades/export_configs_to_helm.py` around lines 94 - 99, The fetch function builds a URL from user input and calls urllib.request.urlopen without validating the scheme; update fetch to parse the URL (use urllib.parse.urlparse or urllib.parse.urlsplit) and explicitly allow only 'http' or 'https' schemes before calling urlopen, raising a clear error (or returning a failure) if the scheme is missing or not allowed; keep the rest of fetch (Request creation, headers, timeout, response handling) unchanged and reference the fetch function when applying this guard.
238-240:⚠️ Potential issue | 🟠 Major
--include-defaultsstill has no effect.The flag is parsed and documented, but nothing in the export path consults
args.include_defaultsbefore populatingconfigs. The output is therefore identical with and without the switch.Also applies to: 258-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/upgrades/export_configs_to_helm.py` around lines 238 - 240, The --include-defaults flag is parsed into args.include_defaults but never used; update the code path that builds/populates the configs list so it conditionally excludes default chart items when args.include_defaults is False. Locate where configs is constructed (the function or block that gathers/applies chart roles/templates) and either pass args.include_defaults into that helper or filter configs afterwards using a predicate like is_default_config(config) to skip defaults; ensure the logic references args.include_defaults and the configs variable so the exported output differs when the flag is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/upgrades/export_configs_to_helm.py`:
- Around line 2-3: The SPDX copyright header line at the top of the module
exceeds 100 characters but is missing the repository’s convention to append the
pylint disable comment; update the top-of-file header (the triple-quoted SPDX
line) to match the repo’s long-header format by adding " # pylint:
disable=line-too-long" to the end of that long copyright line so it remains on
one line and satisfies pylint.
In `@src/service/core/config/configmap_loader.py`:
- Around line 287-346: The loops assume pool_data['common_pod_template'],
['common_resource_validations'], ['common_group_templates'] and ['platforms']
are the right types and crash on null/strings; normalize these fields before
iterating by coercing invalid values to safe defaults (e.g. if not
isinstance(pool_data.get('common_pod_template'), list):
pool_data['common_pod_template'] = [] and similarly ensure
common_resource_validations is a list, common_group_templates is a list, and
platforms is a dict), then proceed with the existing logic (affects the blocks
that build base_pod_template, base_resource_validations, merged_by_key and the
loop that calls _resolve_platform_fields); also apply the same normalization for
the analogous code region around lines 367-405 so reloads fail validation
cleanly instead of raising.
- Around line 381-403: The platform-level override loops silently ignore unknown
names; update the loops that iterate platform_data.get('override_pod_template',
[]) and platform_data.get('resource_validations', []) to emit warnings when a
referenced template or validation is missing (just like the pool-level logic
does). Specifically, when template_name is not in pod_templates, log a warning
referencing platform_data['name'] (or other platform identifier) and the missing
template_name before skipping; likewise when validation_name is not in
resource_validations, log a warning naming the platform and validation_name.
Keep the existing behavior of applying found entries to platform_pod_template
and platform_resource_validations, but ensure every missing reference produces a
clear warning so typos don’t silently fall back to pool defaults.
---
Duplicate comments:
In `@deployments/upgrades/export_configs_to_helm.py`:
- Around line 288-310: The exported config section keys are using camelCase so
the loader (configmap_loader.py) drops them; update the assignments after
calling export_named_configs (for 'pod_template', 'resource_validation',
'group_template', 'backend_test') to set snake_case keys on the configs dict
(use configs['pod_templates'], configs['resource_validations'],
configs['group_templates'], configs['backend_tests'] respectively) while keeping
the export_named_configs calls and variables (pod_templates,
resource_validations, group_templates, backend_tests) unchanged.
- Around line 94-99: The fetch function builds a URL from user input and calls
urllib.request.urlopen without validating the scheme; update fetch to parse the
URL (use urllib.parse.urlparse or urllib.parse.urlsplit) and explicitly allow
only 'http' or 'https' schemes before calling urlopen, raising a clear error (or
returning a failure) if the scheme is missing or not allowed; keep the rest of
fetch (Request creation, headers, timeout, response handling) unchanged and
reference the fetch function when applying this guard.
- Around line 238-240: The --include-defaults flag is parsed into
args.include_defaults but never used; update the code path that builds/populates
the configs list so it conditionally excludes default chart items when
args.include_defaults is False. Locate where configs is constructed (the
function or block that gathers/applies chart roles/templates) and either pass
args.include_defaults into that helper or filter configs afterwards using a
predicate like is_default_config(config) to skip defaults; ensure the logic
references args.include_defaults and the configs variable so the exported output
differs when the flag is set.
🪄 Autofix (Beta)
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: Pro Plus
Run ID: 212cda82-2532-48c7-b82c-f3e1ab154ed8
📒 Files selected for processing (4)
deployments/upgrades/6_2_to_6_3_upgrade.mddeployments/upgrades/export_configs_to_helm.pysrc/service/core/config/configmap_loader.pysrc/service/core/config/tests/test_configmap_loader_unit.py
✅ Files skipped from review due to trivial changes (1)
- deployments/upgrades/6_2_to_6_3_upgrade.md
- export script: add copyright pylint disable, URL scheme validation, remove unused --include-defaults flag, fix f-string without placeholders, add comment explaining camelCase keys (Helm values convention) - configmap_loader: normalize pool/platform list fields before iterating (prevents crashes on null/wrong types), add warning logs for missing platform-level template/validation references Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deployments/upgrades/export_configs_to_helm.py (1)
95-230: Add type annotations to functions in the export helpers.Functions
fetch(),export_*(),strip_*(), andcollect_secret_names()are missing parameter and return type annotations. Add explicit signatures (e.g.,fetch(base_url: str, path: str, headers: dict[str, str]) -> dict[str, Any] | None) to improve clarity and enable type checking, as per the coding guideline requiring strict typing in Python.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/upgrades/export_configs_to_helm.py` around lines 95 - 230, Add explicit type annotations to the helper functions to satisfy the strict-typing guideline: import typing names (Any, Optional, Dict, List, Set) or rely on PEP 604 unions and annotate fetch(base_url: str, path: str, headers: Dict[str, str]) -> Optional[Dict[str, Any]]; strip_fields(data: Any, fields: Set[str] | List[str]) -> Any (or -> Dict[str, Any] | Any to show it may return non-dict unchanged); export_singleton(base_url: str, headers: Dict[str, str], config_type: str, strip: Set[str]) -> Optional[Dict[str, Any]]; export_backends(base_url: str, headers: Dict[str, str]) -> Optional[Dict[str, Dict[str, Any]]]; strip_platform_computed_fields(pool: Dict[str, Any]) -> None; export_pools(base_url: str, headers: Dict[str, str]) -> Optional[Dict[str, Dict[str, Any]]]; export_named_configs(base_url: str, headers: Dict[str, str], path: str) -> Optional[Any]; export_roles(base_url: str, headers: Dict[str, str]) -> Optional[Dict[str, Dict[str, Any]]]; collect_secret_names(configs: Any) -> Set[str]; and add a typed main() -> None if desired; ensure you import Any/Optional/Dict/List/Set (or use builtins like dict[str, Any]) and update any internal variable typing where needed (e.g., backends_list, pools_data) to match the annotations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/upgrades/export_configs_to_helm.py`:
- Around line 253-255: The loop that parses headers (iterating over args.header
and using header.partition(':') to populate headers) currently accepts malformed
inputs like "Authorization" and creates empty values; update it to validate each
header string contains a ':' and that both key.strip() and value.strip() are
non-empty before adding to the headers dict, and if validation fails raise a
clear parser error (e.g., raise argparse.ArgumentTypeError or call parser.error
with "Invalid --header format, expected 'Key: Value'") so malformed headers fail
fast with an explicit message.
---
Nitpick comments:
In `@deployments/upgrades/export_configs_to_helm.py`:
- Around line 95-230: Add explicit type annotations to the helper functions to
satisfy the strict-typing guideline: import typing names (Any, Optional, Dict,
List, Set) or rely on PEP 604 unions and annotate fetch(base_url: str, path:
str, headers: Dict[str, str]) -> Optional[Dict[str, Any]]; strip_fields(data:
Any, fields: Set[str] | List[str]) -> Any (or -> Dict[str, Any] | Any to show it
may return non-dict unchanged); export_singleton(base_url: str, headers:
Dict[str, str], config_type: str, strip: Set[str]) -> Optional[Dict[str, Any]];
export_backends(base_url: str, headers: Dict[str, str]) -> Optional[Dict[str,
Dict[str, Any]]]; strip_platform_computed_fields(pool: Dict[str, Any]) -> None;
export_pools(base_url: str, headers: Dict[str, str]) -> Optional[Dict[str,
Dict[str, Any]]]; export_named_configs(base_url: str, headers: Dict[str, str],
path: str) -> Optional[Any]; export_roles(base_url: str, headers: Dict[str,
str]) -> Optional[Dict[str, Dict[str, Any]]]; collect_secret_names(configs: Any)
-> Set[str]; and add a typed main() -> None if desired; ensure you import
Any/Optional/Dict/List/Set (or use builtins like dict[str, Any]) and update any
internal variable typing where needed (e.g., backends_list, pools_data) to match
the annotations.
🪄 Autofix (Beta)
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: Pro Plus
Run ID: 03248c68-0b2c-4cb9-8fc9-9a997512f2b3
📒 Files selected for processing (2)
deployments/upgrades/export_configs_to_helm.pysrc/service/core/config/configmap_loader.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/service/core/config/configmap_loader.py
jiaenren
approved these changes
Apr 17, 2026
fernandol-nvidia
approved these changes
Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts/export_configs_to_helm.py) stripsparsed_*fields from platforms, producing compact outputConfigMap loader changes (
configmap_loader.py)_resolve_pool_computed_fields(): computesparsed_pod_template,parsed_resource_validations,parsed_group_templatesfor each pool and platform(apiVersion, kind, metadata.name)key dedup, matchingPool.calculate_group_templates()_get_default_mounts(): derivesdefault_mountsfrom resolved pod template, matchingPool.get_default_mounts()Export script (
scripts/export_configs_to_helm.py)PLATFORM_COMPUTED_FIELDSset +strip_platform_computed_fields()to stripparsed_*from platforms within poolsTest plan
test_watcher_resolves_pool_parsed_fieldsbazel test //src/service/core/config/tests:test_configmap_loader_unit— PASSEDbazel test //src/service/core/config:config-pylint— PASSEDbazel test //src/service/core/config/tests:test_configmap_loader_unit-pylint— PASSED🤖 Generated with Claude Code
Checklist
Issue - None
Summary by CodeRabbit
New Features
Documentation