Allow specifying client config fully from envvars#504
Allow specifying client config fully from envvars#504mangelajo merged 1 commit intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes refactor client configuration management to use Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment Variables
participant Settings as ClientConfigV1Alpha1 (BaseSettings)
participant Drivers as ClientConfigV1Alpha1Drivers
participant App as Application
App->>Settings: Instantiate (loads config from env)
Settings->>Drivers: Instantiate (loads drivers from env)
Drivers->>Env: Read JMP_DRIVERS_ALLOW, JMP_DRIVERS_UNSAFE, etc.
Drivers-->>Settings: Return drivers config
Settings-->>App: Return full client config
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py(2 hunks)packages/jumpstarter/jumpstarter/config/client.py(5 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(0 hunks)packages/jumpstarter/jumpstarter/config/env.py(1 hunks)packages/jumpstarter/jumpstarter/config/user_config_test.py(2 hunks)packages/jumpstarter/jumpstarter/utils/env.py(2 hunks)packages/jumpstarter/pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- packages/jumpstarter/jumpstarter/config/client_config_test.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/jumpstarter/jumpstarter/utils/env.py (2)
packages/jumpstarter/jumpstarter/config/client.py (1)
ClientConfigV1Alpha1Drivers(37-54)packages/jumpstarter/jumpstarter/client/client.py (1)
client_from_path(16-20)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (11)
packages/jumpstarter/pyproject.toml (1)
21-21: LGTM! Dependency addition supports the BaseSettings refactoring.The addition of
pydantic-settings>=2.9.1is necessary to support the migration frompydantic.BaseModeltopydantic_settings.BaseSettingsfor enhanced environment variable configuration support.packages/jumpstarter/jumpstarter/config/user_config_test.py (1)
63-63: LGTM! Test updates align with model field renaming.The changes from
name="testclient"toalias="testclient"correctly update the test mocks to match the field renaming in the refactoredClientConfigV1Alpha1model.Also applies to: 90-90
packages/jumpstarter/jumpstarter/utils/env.py (2)
7-7: LGTM! Import updated to use the new settings model.The import change from
_allow_from_envtoClientConfigV1Alpha1Driversaligns with the refactoring to use pydantic settings for environment variable handling.
24-32: LGTM! Improved environment variable handling with pydantic settings.The replacement of manual environment parsing with
ClientConfigV1Alpha1Drivers()instantiation leverages the robust validation and parsing capabilities of pydantic settings. TheClientConfigV1Alpha1Driversclass handles comma-separated string parsing and automaticunsafeflag detection, providing better error handling and validation than the previous manual approach.packages/jumpstarter/jumpstarter/config/env.py (1)
4-8:Details
✅ Verification successful
Environment variable renaming looks good and aligns with the new settings approach.
The updated environment variable names with the
JMP_CLIENT_prefix are consistent with theBaseSettingsconfiguration inclient.pyand provide better clarity about their purpose.However, please ensure that any existing documentation, deployment scripts, or user configurations that reference the old environment variable names are updated accordingly, as this is a breaking change.
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old environment variable names in the codebase # Expected: No references to old names like JMP_NAMESPACE, JMP_NAME, etc. (excluding this env.py file) echo "Searching for old environment variable names..." rg -g '!*/config/env.py' '\b(JMP_NAMESPACE|JMP_NAME|JMP_ENDPOINT|JMP_TOKEN|JMP_DRIVERS_ALLOW)\b' --type pyLength of output: 4122
Environment variable renaming approved – no code changes required
- All internal code (tests, utilities, etc.) imports and uses the updated constants (
JMP_NAMESPACE,JMP_NAME,JMP_ENDPOINT,JMP_TOKEN,JMP_DRIVERS_ALLOW), so nothing else needs updating in the repo.- No stray references to the old environment‐variable literals remain in the codebase.
- Reminder: you’ll still need to update any external documentation, deployment scripts or user configurations that hard-code the old names to use the new
JMP_CLIENT_*keys.packages/jumpstarter/jumpstarter/config/client.py (6)
7-7: Import changes look appropriate for the BaseSettings migration.The addition of
pydantic_settingsimports and the updated pydantic imports are correctly aligned with the refactoring to useBaseSettings.Also applies to: 12-13
37-55: Excellent implementation of environment variable parsing with validation.The
ClientConfigV1Alpha1Driversclass properly extendsBaseSettingsand includes well-designed validators:
decode_allowcorrectly parses comma-separated strings into listsdecode_unsafeproperly detects the "UNSAFE" flag from the allow list- The use of
NoDecodeannotation prevents automatic decoding before custom validation
60-63: Model configuration is well-designed for environment variable support.The
SettingsConfigDictwithenv_prefix="JMP_CLIENT_"andenv_nested_delimiter="_"properly aligns with the updated environment variable names and enables automatic parsing of nested configuration from environment variables.
71-71: Good use of default factories for mutable defaults.Using
Field(default_factory=...)formetadataanddriversfields is the correct approach to avoid shared mutable defaults across instances.Also applies to: 78-78
214-214: Simplifiedfrom_env()method is appropriate.The simplified implementation that just returns
cls()is correct becauseBaseSettingsautomatically handles environment variable loading during instantiation.
279-279: BaseSettings migration for ClientConfigListV1Alpha1 is consistent.The update to extend
BaseSettingsinstead ofBaseModelis consistent with the overall refactoring approach and maintains the existing model configuration.Also applies to: 291-291
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Is there a way to use explicit env var names for fields?
There is validation_alias, I'll try those. |
There is, but it does work as what we would expect, e.g. I can set |
NickCao
force-pushed
the
env-settings
branch
2 times, most recently
from
07f5e77 to
e9db9b2
Compare
| assert config.metadata.name == "testclient" | ||
| assert config.token == "dGhpc2lzYXRva2VuLTEyMzQxMjM0MTIzNEyMzQtc2Rxd3Jxd2VycXdlcnF3ZXJxd2VyLTEyMzQxMjM0MTIz" | ||
| assert config.endpoint == "jumpstarter.my-lab.com:1443" | ||
| assert config.drivers.allow == [] |
There was a problem hiding this comment.
Since it would now be ["UNSAFE"], but it's effectively ignored later on (as long as unsafe is True), so we don't really care about it's value.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.6
git worktree add -d .worktree/backport-504-to-release-0.6 origin/release-0.6
cd .worktree/backport-504-to-release-0.6
git switch --create backport-504-to-release-0.6
git cherry-pick -x 336291891d40b6b88be6530dfecf0f7865ea5b76 |
2 participants
Close-Issue: #502
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Chores
pydantic-settingshas been added for improved configuration handling.Tests