feat: default exporter configs to ~/.config/jumpstarter#712
Conversation
📝 WalkthroughWalkthroughAdds user-first exporter config lookup under CONFIG_PATH/"exporters" with legacy fallback /etc/jumpstarter/exporters; updates load/list/save/delete and CLI create/delete, adds tests for precedence/shadowing, documents search locations, and changes E2E tests/helpers to avoid hardcoded /etc paths. ChangesExporter Configuration User-Level Storage with System Fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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. 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: 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 `@python/packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py`:
- Around line 59-66: The warning about a system config taking effect is
currently skipped when output == OutputMode.PATH because it's in the elif
branch; change the control flow so the warning from
ExporterConfigV1Alpha1.exists(alias) (and the resolved path from
ExporterConfigV1Alpha1._resolve_path(alias)) is emitted to stderr regardless of
the OutputMode.PATH branch—i.e., first echo the machine-readable path to stdout
when output == OutputMode.PATH, then separately check
ExporterConfigV1Alpha1.exists(alias) and click.echo the warning with err=True
(using path and the resolved path) so the warning is never suppressed by the
PATH branch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e16c4a9-648d-4662-87d5-660348f5982c
📒 Files selected for processing (6)
python/conftest.pypython/docs/source/getting-started/configuration/files.mdpython/packages/jumpstarter-cli/jumpstarter_cli/config_exporter.pypython/packages/jumpstarter-cli/jumpstarter_cli/config_exporter_test.pypython/packages/jumpstarter/jumpstarter/config/exporter.pypython/packages/jumpstarter/jumpstarter/config/exporter_test.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
@mmahut thanks for the commit, it looks good, just some comments.
Also you need to update the e2e tests as they are failing to locate the files in the system path https://github.com/jumpstarter-dev/jumpstarter/actions/runs/26632549825/job/78506862972?pr=712#step:12:62
Can you update it so we write in /etc/jumpstarter/exporters/ during the tests?, and perhaps make sure that we also test the user based path as well?.
Thanks a lot.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@mangelajo, thank you so much for your review! I've rebased the branch with the requested changes. I believe the rabbit will pass this time, but sadly he's had enough of me for today :) |
raballew
left a comment
raballew
left a comment
There was a problem hiding this comment.
Thank you for tackling this issue. I ran into this issue myself multiple times but now I remember why I never found the time to work on this. save() and delete() are critical findings, the rest is mostly nits.
Co-authored-by: Miguel Angel Ajo Pelayo <majopela@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.py (1)
191-199: ⚡ Quick winAdd a system-only exporter config scenario to cover the new warning branch.
Please add a case with
mock_user_config_exists.return_value = Falseandmock_config_exists.return_value = Trueto assert the warning path is exercised andExporterConfigV1Alpha1.delete(...)is not called.Also applies to: 207-214, 221-229
🤖 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 `@python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.py` around lines 191 - 199, Add a new test-case variant for the delete exporter flow that simulates a system-only exporter config by setting mock_user_config_exists.return_value = False and mock_config_exists.return_value = True, invoke the CLI delete command (delete, ["exporter", EXPORTER_NAME]) and assert the CLI prints the system-only warning message and that ExporterConfigV1Alpha1.delete (mock_config_delete) was NOT called; duplicate the same added scenario for the other groups of assertions currently at the nearby blocks (the ones using mock_delete_exporter, mock_config_delete and runner.invoke) so the warning branch is covered in each of the three places referenced.
🤖 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 `@python/packages/jumpstarter/jumpstarter/config/exporter.py`:
- Around line 196-214: The code incorrectly uses Path.with_suffix() in _get_path
and _resolve_path (and the similar block later) which strips any trailing dot
segments from aliases (e.g., "lab.v2" → "lab.yaml"); change these to construct
the filename by concatenation instead of with_suffix so dots in aliases are
preserved — e.g., validate_alias(alias) then derive paths via cls.BASE_PATH /
f"{alias}.yaml" and (cls.SYSTEM_CONFIG_PATH / f"{alias}.yaml") respectively, and
update the other occurrence (the similar logic at the later block) the same way
so save()/load()/delete()/exists()/list() operate on the correct filenames.
---
Nitpick comments:
In `@python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.py`:
- Around line 191-199: Add a new test-case variant for the delete exporter flow
that simulates a system-only exporter config by setting
mock_user_config_exists.return_value = False and mock_config_exists.return_value
= True, invoke the CLI delete command (delete, ["exporter", EXPORTER_NAME]) and
assert the CLI prints the system-only warning message and that
ExporterConfigV1Alpha1.delete (mock_config_delete) was NOT called; duplicate the
same added scenario for the other groups of assertions currently at the nearby
blocks (the ones using mock_delete_exporter, mock_config_delete and
runner.invoke) so the warning branch is covered in each of the three places
referenced.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6385624a-4fcf-4e8a-aa6f-0467c5134b3f
📒 Files selected for processing (14)
e2e/test/compat_old_client_test.goe2e/test/compat_old_controller_test.goe2e/test/e2e_test.goe2e/test/go.mode2e/test/hooks_test.goe2e/test/utils.gopython/conftest.pypython/docs/source/getting-started/configuration/files.mdpython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/config_exporter.pypython/packages/jumpstarter-cli/jumpstarter_cli/config_exporter_test.pypython/packages/jumpstarter/jumpstarter/config/exporter.pypython/packages/jumpstarter/jumpstarter/config/exporter_test.py
🚧 Files skipped from review as they are similar to previous changes (8)
- python/docs/source/getting-started/configuration/files.md
- python/conftest.py
- e2e/test/compat_old_controller_test.go
- e2e/test/hooks_test.go
- e2e/test/go.mod
- e2e/test/utils.go
- e2e/test/compat_old_client_test.go
- e2e/test/e2e_test.go
| def _get_path(cls, alias: str) -> Path: | ||
| cls.validate_alias(alias) | ||
| return (cls.BASE_PATH / alias).with_suffix(".yaml") | ||
|
|
||
| @classmethod | ||
| def exists(cls, alias: str): | ||
| def _resolve_path(cls, alias: str) -> Path: | ||
| """Return an alias's config path, preferring the user dir over the production system location. | ||
|
|
||
| Falls back to the system location only when the user-dir file does not exist. When neither | ||
| exists, the user-dir path is returned so callers raise a ``FileNotFoundError`` pointing at | ||
| the current default location. | ||
| """ | ||
| user_path = cls._get_path(alias) | ||
| if user_path.exists(): | ||
| return user_path | ||
| system_path = (cls.SYSTEM_CONFIG_PATH / alias).with_suffix(".yaml") | ||
| if system_path.exists(): | ||
| return system_path | ||
| return user_path |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
for alias in ("lab", "lab.v2", "lab.test.v3"):
print(f"{alias} -> {(Path('/tmp') / alias).with_suffix('.yaml').name}")
PY
rg -n 'with_suffix\("\.yaml"\)' python/packages/jumpstarter/jumpstarter/config/exporter.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect validate_alias + surrounding path logic in exporter.py
python - <<'PY'
import itertools, pathlib
p = pathlib.Path("python/packages/jumpstarter/jumpstarter/config/exporter.py")
lines = p.read_text().splitlines()
for i in range(1, len(lines)+1):
if "def validate_alias" in lines[i-1]:
start=max(1,i-20)
end=min(len(lines), i+80)
for n in range(start, end+1):
print(f"{n:4d}:{lines[n-1]}")
break
PY
# 2) Find usages of _resolve_path/_get_path and key round-trip operations
rg -n "_resolve_path\(|_get_path\(|validate_alias\(" python/packages/jumpstarter/jumpstarter/config/exporter.py
rg -n "save\(|load\(|delete\(|exists\(" python/packages/jumpstarter/jumpstarter/config -S
# 3) Find CLI existence guard mentioning config paths / aliases
rg -n "alias|config.*path|FileNotFoundError|exists\(" python/packages/jumpstarter -S
# 4) Show definitions of save/load/delete (if present)
rg -n "def (save|load|delete)\b" python/packages/jumpstarter -SRepository: jumpstarter-dev/jumpstarter
Length of output: 40981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ExporterConfigV1Alpha1\.(exists|load|save|delete)" python/packages/jumpstarter -S
rg -n "exporter.*exists\(" python/packages/jumpstarter -S
rg -n "\.exists\(\s*[^)]*alias" python/packages/jumpstarter -S
rg -n "FileNotFoundError.*Exporter|ConfigurationError.*Exporter|CLI.*Exporter" python/packages/jumpstarter -SRepository: jumpstarter-dev/jumpstarter
Length of output: 1889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show exporter.py sections relevant to collisions: list(), save(), delete()
python - <<'PY'
from pathlib import Path
p = Path("python/packages/jumpstarter/jumpstarter/config/exporter.py")
lines = p.read_text().splitlines()
def show(start, end):
for i in range(start, end+1):
print(f"{i:4d}:{lines[i-1]}")
show(240, 310) # list/save/delete
PY
# Inspect exporter_test: validate_alias coverage and any dot-alias tests
python - <<'PY'
from pathlib import Path
p = Path("python/packages/jumpstarter/jumpstarter/config/exporter_test.py")
lines = p.read_text().splitlines()
for i,l in enumerate(lines, start=1):
if "test_alias_path_traversal_rejected" in l:
start=max(1,i-5); end=min(len(lines), i+35)
for j in range(start,end+1):
print(f"{j:4d}:{lines[j-1]}")
break
PY
rg -n "lab\.v|v2|\\.[a-zA-Z0-9_-]+" python/packages/jumpstarter/jumpstarter/config/exporter_test.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 9922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ExporterConfigV1Alpha1\.(exists|load|save|delete|user_config_exists)" python/packages/jumpstarter -S
# Look for CLI/existence guard patterns around exporters
rg -n "exporter" python/packages/jumpstarter -S | rg -n "(exists|FileNotFoundError|load|delete|config)" || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 25940
Stop using Path.with_suffix() to derive config filenames.
ExporterConfigV1Alpha1.validate_alias() allows dots in aliases (only blocks path separators and . / ..), but with_suffix(".yaml") truncates everything after the last . (e.g. lab.v2 → lab.yaml). This makes distinct aliases collide, breaking save()/load()/delete()/exists() and causing list() (which uses entry.stem) to report/load the wrong alias/file.
Proposed fix
`@classmethod`
def _get_path(cls, alias: str) -> Path:
cls.validate_alias(alias)
- return (cls.BASE_PATH / alias).with_suffix(".yaml")
+ return cls.BASE_PATH / f"{alias}.yaml"
@@
- system_path = (cls.SYSTEM_CONFIG_PATH / alias).with_suffix(".yaml")
+ system_path = cls.SYSTEM_CONFIG_PATH / f"{alias}.yaml"
@@
- system_path = (cls.SYSTEM_CONFIG_PATH / alias).with_suffix(".yaml")
+ system_path = cls.SYSTEM_CONFIG_PATH / f"{alias}.yaml"Also applies to: 293-299
🤖 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 `@python/packages/jumpstarter/jumpstarter/config/exporter.py` around lines 196
- 214, The code incorrectly uses Path.with_suffix() in _get_path and
_resolve_path (and the similar block later) which strips any trailing dot
segments from aliases (e.g., "lab.v2" → "lab.yaml"); change these to construct
the filename by concatenation instead of with_suffix so dots in aliases are
preserved — e.g., validate_alias(alias) then derive paths via cls.BASE_PATH /
f"{alias}.yaml" and (cls.SYSTEM_CONFIG_PATH / f"{alias}.yaml") respectively, and
update the other occurrence (the similar logic at the later block) the same way
so save()/load()/delete()/exists()/list() operate on the correct filenames.
3 participants
Fixes #76.
Exporter configs created via the CLI now default to
~/.config/jumpstarter/exporters/(consistent with clients), instead of the hardcoded/etc/jumpstarter/exporters/which needs root./etcis kept as a read fallback, so existing systemd/container deployments keep working.