Add exporter context env vars to jmp shell and env_with_metadata() helper#291
Open
Add exporter context env vars to jmp shell and env_with_metadata() helper#291
Conversation
Contributor
📝 WalkthroughWalkthroughAdds exporter metadata propagation: new ExporterMetadata and env_with_metadata helpers; new JMP_EXPORTER and JMP_EXPORTER_LABELS env constants; Lease stores exporter_labels and fetches them after acquisition; launch_shell merges lease-derived env vars; tests added for parsing and shell behavior. Changes
Sequence DiagramsequenceDiagram
participant User
participant EnvSync as env_with_metadata()
participant Portal as BlockingPortal
participant EnvAsync as env_with_metadata_async()
participant Client
participant Lease
participant OS as Environment
participant Meta as ExporterMetadata
User->>EnvSync: enter context
EnvSync->>Portal: create blocking portal
EnvSync->>EnvAsync: call async helper with portal
EnvAsync->>Client: create client via env_async()
EnvAsync->>OS: read JMP_EXPORTER, JMP_EXPORTER_LABELS, JMP_LEASE
OS-->>Meta: provide raw vars
Meta->>Meta: ExporterMetadata.from_env()
EnvAsync-->>EnvSync: yield (Client, ExporterMetadata)
EnvSync-->>User: provide (client, metadata)
User->>Client: use client (may request lease)
Client->>Lease: acquire()
Lease->>Lease: set exporter_name from acquisition result
Lease->>Lease: call _fetch_exporter_labels()
Lease->>OS: provide labels for shell env (JMP_EXPORTER, JMP_LEASE, JMP_EXPORTER_LABELS)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 |
mangelajo
requested review from
NickCao,
evakhoni and
kirkbrauer
and removed request for
kirkbrauer
mangelajo
force-pushed
the
feat/exporter-metadata-env
branch
from
555b95a to
3f784d1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/common/utils_test.py (1)
29-45:test_launch_shell_sets_lease_envcurrently doesn’t validate env propagation.It only asserts process success; it should also assert that the three lease/exporter env vars are actually present.
Suggested test strengthening
-def test_launch_shell_sets_lease_env(tmp_path, monkeypatch): +def test_launch_shell_sets_lease_env(tmp_path, monkeypatch, capsys): monkeypatch.setenv("SHELL", shutil.which("env")) lease = SimpleNamespace( exporter_name="my-exporter", name="lease-123", exporter_labels={"board": "rpi4", "location": "lab-1"}, lease_ending_callback=None, ) exit_code = launch_shell( host=str(tmp_path / "test.sock"), context="my-exporter", allow=["*"], unsafe=False, use_profiles=False, lease=lease, ) assert exit_code == 0 + out = capsys.readouterr().out + assert "JMP_EXPORTER=my-exporter" in out + assert "JMP_LEASE=lease-123" in out + assert "JMP_EXPORTER_LABELS=board=rpi4,location=lab-1" in out🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/common/utils_test.py` around lines 29 - 45, The test only checks exit code but must also verify the launched shell received lease-related env vars: update test_launch_shell_sets_lease_env to capture the launched process stdout (e.g., use capsys or patch subprocess.run/capture_output) when SHELL is set to the env binary and assert the output contains the lease.exporter_name, lease.name, and each key=value from lease.exporter_labels so you verify the three lease/exporter env vars are actually propagated by launch_shell.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter/jumpstarter/client/lease.py`:
- Around line 142-149: The _fetch_exporter_labels method can leave stale
self.exporter_labels when svc.GetExporter fails; modify the exception path in
_fetch_exporter_labels (the async method) to clear or reset self.exporter_labels
(e.g., set to {} or None) when an exception occurs and include the exporter name
in the debug log (use logger.debug("Could not fetch labels for exporter %s: %s",
self.exporter_name, e) or similar) so reused lease instances do not retain
previous labels.
In `@python/packages/jumpstarter/jumpstarter/common/utils_test.py`:
- Around line 70-75: The test
test_exporter_metadata_from_env_labels_with_equals_in_value triggers typo-linter
noise because of the sample value; update the fixture to use a less typo-like
sample while preserving the edge case (an equals sign inside a label value),
e.g. replace "key=val=ue,other=ok" with something like "key=a=b,other=ok", keep
the monkeypatch calls and the assertion that ExporterMetadata.from_env() yields
meta.labels == {"key": "a=b", "other": "ok"}.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/common/utils_test.py`:
- Around line 29-45: The test only checks exit code but must also verify the
launched shell received lease-related env vars: update
test_launch_shell_sets_lease_env to capture the launched process stdout (e.g.,
use capsys or patch subprocess.run/capture_output) when SHELL is set to the env
binary and assert the output contains the lease.exporter_name, lease.name, and
each key=value from lease.exporter_labels so you verify the three lease/exporter
env vars are actually propagated by launch_shell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 250a74dd-bc0f-4834-8909-f4326472e1c6
📒 Files selected for processing (5)
python/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/common/utils_test.pypython/packages/jumpstarter/jumpstarter/config/env.pypython/packages/jumpstarter/jumpstarter/utils/env.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/utils/env.py (1)
13-35: Consider using immutable mapping forlabelsfield to strengthen frozen dataclass contract.
ExporterMetadatais markedfrozen=Trueto indicate immutable semantics, butlabels: dict[str, str]can still be mutated via direct assignment (e.g.,metadata.labels['key'] = 'value'). SinceExporterMetadatais exported as public API, wrapping it withMappingProxyTypewould prevent accidental mutation and clarify intent.♻️ Suggested refactor
import os from contextlib import ExitStack, asynccontextmanager, contextmanager from dataclasses import dataclass, field +from types import MappingProxyType +from typing import Mapping @@ `@dataclass`(frozen=True) class ExporterMetadata: @@ - labels: dict[str, str] = field(default_factory=dict) + labels: Mapping[str, str] = field(default_factory=lambda: MappingProxyType({})) @@ - return cls(name=name, labels=labels, lease=lease) + return cls(name=name, labels=MappingProxyType(labels), lease=lease)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/utils/env.py` around lines 13 - 35, ExporterMetadata is frozen but exposes a mutable dict via the labels field; change labels to an immutable mapping and wrap any dicts produced in from_env with types.MappingProxyType (or change the annotation to Mapping[str,str] and return MappingProxyType(labels)) so callers cannot mutate the labels after construction; update the dataclass field default to produce an immutable empty mapping via MappingProxyType({}) (and add the import for MappingProxyType and typing.Mapping if needed) while keeping the class name ExporterMetadata and the factory method from_env unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/utils/env.py`:
- Around line 13-35: ExporterMetadata is frozen but exposes a mutable dict via
the labels field; change labels to an immutable mapping and wrap any dicts
produced in from_env with types.MappingProxyType (or change the annotation to
Mapping[str,str] and return MappingProxyType(labels)) so callers cannot mutate
the labels after construction; update the dataclass field default to produce an
immutable empty mapping via MappingProxyType({}) (and add the import for
MappingProxyType and typing.Mapping if needed) while keeping the class name
ExporterMetadata and the factory method from_env unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96ff34ba-0502-43d4-b638-ac73844f7698
📒 Files selected for processing (5)
python/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/common/utils_test.pypython/packages/jumpstarter/jumpstarter/config/env.pypython/packages/jumpstarter/jumpstarter/utils/env.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter/jumpstarter/common/utils_test.py
- python/packages/jumpstarter/jumpstarter/client/lease.py
- python/packages/jumpstarter/jumpstarter/common/utils.py
Contributor
I wonder what would happen with complex selectors. are we fetching the selector we leased with, or the labels of the exporter that ended up matching that selector? |
mangelajo
force-pushed
the
feat/exporter-metadata-env
branch
from
3f784d1 to
39b3926
Compare
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…lper Set JMP_EXPORTER, JMP_LEASE, and JMP_EXPORTER_LABELS environment variables when launching a jmp shell with a remote lease. Add env_with_metadata() context manager that yields (client, ExporterMetadata) so scripts can access the exporter name, labels, and lease ID from within a shell session. Closes #53 Made-with: Cursor
mangelajo
force-pushed
the
feat/exporter-metadata-env
branch
from
39b3926 to
8b28485
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/common/utils_test.py (1)
29-45: Consider verifying the environment variables are actually set.The test confirms
launch_shellaccepts a lease and exits successfully, but doesn't verify thatJMP_EXPORTER,JMP_LEASE, andJMP_EXPORTER_LABELSare actually propagated to the subprocess environment.🧪 Optional enhancement to capture and verify env vars
def test_launch_shell_sets_lease_env(tmp_path, monkeypatch): - monkeypatch.setenv("SHELL", shutil.which("env")) + # Use a script that outputs specific env vars to a file + env_output = tmp_path / "env_output.txt" + script = tmp_path / "capture_env.sh" + script.write_text(f"""#!/bin/sh +echo "JMP_EXPORTER=$JMP_EXPORTER" >> {env_output} +echo "JMP_LEASE=$JMP_LEASE" >> {env_output} +echo "JMP_EXPORTER_LABELS=$JMP_EXPORTER_LABELS" >> {env_output} +""") + script.chmod(0o755) + monkeypatch.setenv("SHELL", str(script)) lease = SimpleNamespace( exporter_name="my-exporter", name="lease-123", exporter_labels={"board": "rpi4", "location": "lab-1"}, lease_ending_callback=None, ) exit_code = launch_shell( host=str(tmp_path / "test.sock"), context="my-exporter", allow=["*"], unsafe=False, use_profiles=False, lease=lease, ) assert exit_code == 0 + output = env_output.read_text() + assert "JMP_EXPORTER=my-exporter" in output + assert "JMP_LEASE=lease-123" in output + assert "board=rpi4" in output + assert "location=lab-1" in output🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/common/utils_test.py` around lines 29 - 45, Update test_launch_shell_sets_lease_env to verify the env vars passed to the child process by mocking the subprocess invocation used inside launch_shell (e.g., subprocess.run or subprocess.Popen depending on implementation) so you can capture the env dict passed to it; call launch_shell with the same lease fixture and then assert the captured env contains keys/values for JMP_EXPORTER == lease.exporter_name, JMP_LEASE == lease.name, and JMP_EXPORTER_LABELS == json.dumps(lease.exporter_labels) (or the exact serialization used by launch_shell). Ensure you restore/cleanup the mock so other tests are unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/common/utils_test.py`:
- Around line 29-45: Update test_launch_shell_sets_lease_env to verify the env
vars passed to the child process by mocking the subprocess invocation used
inside launch_shell (e.g., subprocess.run or subprocess.Popen depending on
implementation) so you can capture the env dict passed to it; call launch_shell
with the same lease fixture and then assert the captured env contains
keys/values for JMP_EXPORTER == lease.exporter_name, JMP_LEASE == lease.name,
and JMP_EXPORTER_LABELS == json.dumps(lease.exporter_labels) (or the exact
serialization used by launch_shell). Ensure you restore/cleanup the mock so
other tests are unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59d1ab79-7ea0-470d-844b-24ea3e260ccd
📒 Files selected for processing (5)
python/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/common/utils_test.pypython/packages/jumpstarter/jumpstarter/config/env.pypython/packages/jumpstarter/jumpstarter/utils/env.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/common/utils.py (1)
87-97: Consider adding type hint for theleaseparameter.The function works correctly but lacks a type annotation for
lease. Adding a type hint would improve IDE support and documentation.📝 Suggested type annotation
-def _lease_env_vars(lease) -> dict[str, str]: +def _lease_env_vars(lease: "Lease") -> dict[str, str]:You would need to add the import under
TYPE_CHECKING:if TYPE_CHECKING: from jumpstarter.driver import Driver + from jumpstarter.client.lease import Lease🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/common/utils.py` around lines 87 - 97, The _lease_env_vars function is missing a type annotation for the lease parameter; update its signature to include the lease type (e.g., lease: Lease) and add a conditional import for that type under TYPE_CHECKING (from typing import TYPE_CHECKING; if TYPE_CHECKING: from <module> import Lease) so you don't create runtime import cycles; update any forward references if needed and keep existing logic using JMP_EXPORTER, JMP_LEASE, and JMP_EXPORTER_LABELS unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/common/utils.py`:
- Around line 87-97: The _lease_env_vars function is missing a type annotation
for the lease parameter; update its signature to include the lease type (e.g.,
lease: Lease) and add a conditional import for that type under TYPE_CHECKING
(from typing import TYPE_CHECKING; if TYPE_CHECKING: from <module> import Lease)
so you don't create runtime import cycles; update any forward references if
needed and keep existing logic using JMP_EXPORTER, JMP_LEASE, and
JMP_EXPORTER_LABELS unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fea5fdb-d1aa-4ef1-be79-9551a75efed5
📒 Files selected for processing (5)
python/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/common/utils_test.pypython/packages/jumpstarter/jumpstarter/config/env.pypython/packages/jumpstarter/jumpstarter/utils/env.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter/jumpstarter/client/lease.py
- python/packages/jumpstarter/jumpstarter/common/utils_test.py
raballew
requested changes
Mar 18, 2026
Contributor
raballew
left a comment
raballew
left a comment
There was a problem hiding this comment.
The label serialization (key1=val1,key2=val2) will break on Kubernetes label values that contain commas -- e.g. env=dev,staging would get parsed as two separate labels. JSON would handle this cleanly, or at least reject/document the limitation.
Also the test test_launch_shell_sets_lease_env only checks exit_code == 0 but doesn't actually verify the env vars show up in the output. And env_with_metadata() / env_with_metadata_async() and _fetch_exporter_labels don't have tests yet.
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
JMP_EXPORTER,JMP_LEASE, andJMP_EXPORTER_LABELSenvironment variables when launching ajmp shellwith a remote lease, so scripts and commands running inside the shell can discover the exporter context.env_with_metadata()/env_with_metadata_async()context managers andExporterMetadatadataclass that let Python code retrieve the exporter name, labels, and lease ID from within a shell session.Leaseobject.Usage
Or directly from the shell environment:
Closes #53
Test plan
test_launch_shell_sets_lease_envverifies env vars are set when a lease is providedtest_exporter_metadata_from_envverifies parsing with all env vars settest_exporter_metadata_from_env_emptyverifies graceful behavior with no env varstest_exporter_metadata_from_env_labels_with_equals_in_valueverifies label values containing=Made with Cursor
Summary by CodeRabbit
New Features
Tests