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
| // 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.
There was a problem hiding this comment.
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.
If we think that will be clearer, then breaking now probably makes sense. Having these settings be exclusive does reduce the questions around resolution precedence.
I don't want to do that in this PR though. If we think that's a direction we want to go, then let's make that change in a separate PR so that the possibly breaking change is more clearly surfaced.
| } | ||
|
|
||
| #[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
elezar
added
gator:blocked
BlockedGator is blocked by merge conflicts with the base branch. GitHub reports Next action: @elezar, please rebase or merge the base branch and resolve the conflicts, then push an updated head so gator can re-check review state and CI. |
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. Remaining items:
Next state: |
elezar
added
gator:in-review
Maintainer Approval NeededGator validation and PR monitoring are complete. Validation: maintainer-authored, project-valid local Docker e2e and SSH environment stabilization work. Human maintainer approval or merge decision is now required. |
elezar
removed
the
gator:in-review
elezar
added
the
gator:approval-needed
elezar
added
gator:in-review
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved, but not ready to leave review. Remaining items:
Docs: relevant Docker supervisor image behavior remains covered in the Docker driver README and Next state: |
Re-check After CI UpdateI re-evaluated latest head Disposition: partially resolved, but not ready to leave review. Remaining items:
Next state: |
Signed-off-by: Evan Lezar <elezar@nvidia.com>
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