fix(e2e): stabilize local Docker smoke test#1935
Conversation
|
🌿 Preview your docs: https://nvidia-preview-pr-1935.docs.buildwithfern.com/openshell |
elezar
added
test:e2e
|
Label |
|
Label |
PR Review StatusValidation: this is maintainer-authored, project-valid Docker e2e and SSH environment stabilization work. It keeps local Docker e2e aligned with the configured supervisor image and prevents host linker variables from breaking the external OpenSSH process while preserving the proxy child environment. Review findings:
Docs: relevant Docker supervisor image behavior is reflected in the Docker driver README and Next state: |
Re-check After CI UpdateI re-evaluated latest head Disposition: not resolved. Remaining items:
Next state: |
elezar
added
gator:in-review
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
| // Tier 2: explicit supervisor_image in [openshell.drivers.docker]. | ||
| // A configured image should be the source of truth even when a local | ||
| // developer build is present under target/. | ||
| if let Some(image) = docker_config.supervisor_image.clone() { | ||
| return extract_supervisor_bin_from_image(docker, &image).await; | ||
| } |
There was a problem hiding this comment.
There is a comment here:
OpenShell/crates/openshell-driver-docker/src/lib.rs
Lines 159 to 163 in 242ace2
Could be something like:
/// Optional image used to extract the Linux `openshell-sandbox` binary.
/// Ignored when `supervisor_bin` is set. See `resolve_supervisor_bin` for
/// the full resolution order.
pub supervisor_image: Option<String>,
It could make sense to validate the config and only allow either supervisor_bin or supervisor_image, so both cannot be set. But that would be a breaking change.
There was a problem hiding this comment.
Also, it would be good to have a test for this new resolution order.
| } | ||
|
|
||
| #[test] | ||
| #[allow(unsafe_code)] // Test-only: env vars require unsafe in Rust 2024. |
There was a problem hiding this comment.
You could use temp-env here (cli crate already has a dep on it) to remove the unsafe and resetting the env var.
I think it could also remove the need for the lock, based on their docs.
Avoid interference when running concurrently
2 participants
Summary
Fix local Docker e2e by honoring the configured supervisor image and isolating host OpenSSH from Nix/devenv linker variables while preserving the proxy child environment.
Related Issue
N/A
Changes
ssh.openshell ssh-proxychild command.Testing
mise run pre-commitpassesAdditional validation run:
/home/elezar/.cargo/bin/cargo fmt --check/home/elezar/.cargo/bin/cargo test -p openshell-cli linker_environment/home/elezar/.cargo/bin/cargo clippy -p openshell-cli --all-targets -- -D warningsmise run e2e:dockermise run pre-commitwas attempted locally but did not complete:helm lintreported missing chart dependency metadata forpostgresql, and threeopenshell-supervisor-processtests failed because this shell could not resolve the current user/group. The clippy failure from this change was fixed and rechecked.Checklist