Add exporter context env vars to jmp shell and env_with_metadata() helper#291
Add exporter context env vars to jmp shell and env_with_metadata() helper#291
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:
📝 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: start 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: raw env vars
Meta->>Meta: ExporterMetadata.from_env()
EnvAsync-->>EnvSync: yield (Client, ExporterMetadata)
EnvSync-->>User: provide (client, metadata)
User->>Client: request lease
Client->>Lease: acquire()
Lease->>Lease: set exporter_name from result
Lease->>Lease: call _fetch_exporter_labels()
Lease->>OS: read/write exporter labels (for shell env)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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}
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
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
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. |
mangelajo
force-pushed
the
feat/exporter-metadata-env
branch
from
39b3926 to
8b28485
Compare
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
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
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.
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Regarding the comma concern: Kubernetes label values are restricted to alphanumerics, -, _, and . (regex: ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]). Commas are not valid in label keys or values, so env=dev,staging would be rejected by the Kubernetes API. The comma-separated serialization is therefore safe for valid Kubernetes labels.
The points about test coverage are valid though — will look into adding tests for env_with_metadata() / env_with_metadata_async() and _fetch_exporter_labels.
ambient-code
bot
force-pushed
the
feat/exporter-metadata-env
branch
from
8b28485 to
7347901
Compare
mangelajo
dismissed
raballew’s stale review
Kubernetes labels cannot have commas in key or values.
| use_profiles=False, | ||
| lease=lease, | ||
| ) | ||
| assert exit_code == 0 |
There was a problem hiding this comment.
This test proves the shell launched but doesn't actually check that JMP_EXPORTER, JMP_LEASE, and JMP_EXPORTER_LABELS were set to the right values. Since SHELL is set to env, you could capture stdout and assert on the expected env var values - or use a small wrapper script that writes specific vars to a temp file for verification.
There was a problem hiding this comment.
Good catch — rewrote the test to use a capture script that writes the env vars to a temp file, then asserts on JMP_EXPORTER=my-exporter, JMP_LEASE=lease-123, and the label values.
| async def _fetch_exporter_labels(self): | ||
| """Fetch the exporter's labels after lease acquisition.""" | ||
| try: | ||
| exporter = await self.svc.GetExporter(name=self.exporter_name) | ||
| self.exporter_labels = exporter.labels | ||
| except Exception as e: | ||
| self.exporter_labels = {} | ||
| logger.warning("Could not fetch labels for exporter %s: %s", self.exporter_name, e) |
There was a problem hiding this comment.
This has two code paths (success + exception fallback) but neither is directly unit-tested. The happy path only runs during full lease acquisition against a real controller. Would be nice to have a couple of unit tests mocking self.svc.GetExporter - one for success, one for the exception fallback.
There was a problem hiding this comment.
Added test_fetch_exporter_labels_success and test_fetch_exporter_labels_failure — both mock svc.GetExporter. The failure test also verifies stale labels are cleared to {}.
| def _build_common_env( | ||
| host: str, | ||
| allow: list[str], | ||
| unsafe: bool, | ||
| *, | ||
| lease=None, | ||
| insecure: bool = False, | ||
| passphrase: str | None = None, | ||
| ) -> dict[str, str]: | ||
| """Build the base environment dict for shell/command processes.""" | ||
| env = os.environ | { | ||
| JUMPSTARTER_HOST: host, | ||
| JMP_DRIVERS_ALLOW: "UNSAFE" if unsafe else ",".join(allow), | ||
| "_JMP_SUPPRESS_DRIVER_WARNINGS": "1", # Already warned during client initialization | ||
| } | ||
| if insecure: | ||
| env = env | {JMP_GRPC_INSECURE: "1"} | ||
| if passphrase: | ||
| env = env | {JMP_GRPC_PASSPHRASE: passphrase} | ||
| if lease is not None: | ||
| env.update(_lease_env_vars(lease)) | ||
| return env |
There was a problem hiding this comment.
New helper with a decent parameter matrix (insecure, passphrase, lease combos) but it's only tested indirectly through launch_shell(). Direct unit tests for this function would make it easier to catch regressions - especially edge cases like passphrase="" (falsy, so JMP_GRPC_PASSPHRASE won't be set - is that intentional?).
AI-generated, human reviewed
There was a problem hiding this comment.
Added direct unit tests for _build_common_env (minimal, unsafe, insecure, passphrase, empty passphrase, with lease) and _lease_env_vars. Re: passphrase="" — it's intentionally falsy so JMP_GRPC_PASSPHRASE won't be set, consistent with the str | None typing. Added a test to document this behavior.
| for pair in raw_labels.split(","): | ||
| if "=" in pair: | ||
| k, v = pair.split("=", 1) | ||
| labels[k] = v |
There was a problem hiding this comment.
Minor: if JMP_EXPORTER_LABELS contains something like =value, this creates {"": "value"} since split("=", 1) yields ["", "value"]. Unlikely with k8s labels but a quick if k: guard before the assignment would be cleaner.
There was a problem hiding this comment.
The labels really come from k8s, they cannot have that form :) but an "if k:" really won't hurt, I am buying :)
There was a problem hiding this comment.
Done — added the if k: guard. Also added a test (test_exporter_metadata_from_env_ignores_empty_key) to cover this case.
ambient-code
bot
force-pushed
the
feat/exporter-metadata-env
branch
3 times, most recently
from
1b0b739 to
c209fb5
Compare
There was a problem hiding this comment.
Addressed all review comments in c209fb5:
env.pyempty-key guard: Addedif k:guard in label parsing as suggestedutils_test.pyenv var verification: Rewrotetest_launch_shell_sets_lease_envto capture env vars via a shell script and assert onJMP_EXPORTER,JMP_LEASE, andJMP_EXPORTER_LABELSvalueslease.py_fetch_exporter_labelstests: Addedtest_fetch_exporter_labels_successandtest_fetch_exporter_labels_failurethat mocksvc.GetExporterfor both code paths (also verifies stale labels are cleared to{})utils.py_build_common_envtests: Added direct unit tests covering minimal, unsafe, insecure, passphrase, empty passphrase, and lease scenarios. Also added tests for_lease_env_vars. Note: empty string passphrase ("") is intentionally falsy soJMP_GRPC_PASSPHRASEwon't be set — this is consistent with howpassphraseis handled elsewhere (typed asstr | None).
ambient-code
bot
force-pushed
the
feat/exporter-metadata-env
branch
from
c209fb5 to
7cb3d5a
Compare
|
@ambient-code please rebase this |
…lper Set JMP_EXPORTER, JMP_LEASE, and JMP_EXPORTER_LABELS environment variables when launching a shell with a remote lease. Add ExporterMetadata dataclass and env_with_metadata() context managers for retrieving exporter context from within a shell session. Fetch exporter labels from the controller during lease acquisition. Closes #53 Co-Authored-By: Miguel Angel Ajo Pelayo <miguelangel@ajo.es> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ambient-code
bot
force-pushed
the
feat/exporter-metadata-env
branch
from
7cb3d5a to
91ca0d3
Compare
3 participants
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