Repository navigation
Conversation
|
🌿 Preview your docs: https://nvidia-preview-pr-4424.docs.buildwithfern.com/openshell |
|
Label |
Signed-off-by: Evan Lezar <elezar@nvidia.com>
4450756 to
930f026
Compare
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
matthewgrossman
left a comment
There was a problem hiding this comment.
LGTM. I verified the restored coverage in the latest CI run: both Kubernetes jobs (v1beta1 and v1alpha1) report 96 tests run: 96 passed, 4 skipped, including the single profile.e2e-kubernetes.default-filter exclusion. The "Final GitHub Kubernetes run pending" testing item can be checked off.
I also confirmed that the intentional conformance-only lanes still run only conformance. The workflow exports an empty OPENSHELL_E2E_KUBERNETES_FEATURES, but the mise task's e2e-conformance-only value takes precedence; the kubernetes-external-driver-e2e log shows conformance targets only. No other workflow or task relies on the old empty-value behavior.
Optional: the branch predates #4081, so this run did not include the new k3s-ha conformance lane. The changes don't overlap, but a rebase would give a green run alongside it before merge.
Non-blocking suggestions inline.
| run_e2e() { | ||
| run_conformance | ||
| if [ -z "${E2E_FEATURES}" ]; then | ||
| if [ "${E2E_FEATURES}" = "e2e-conformance-only" ]; then |
There was a problem hiding this comment.
Non-blocking: the original gap was a lane that skipped its Rust tests while reporting success. With this change the default is correct, but a future workflow or task change could narrow the run again without anyone noticing. Could we make the skip visible here and in the other three runners? For example, echo "::notice::Skipping Rust e2e tests (e2e-conformance-only)". A follow-up could also fail default lanes that execute zero Rust tests. The mocked selector checks described in the PR body would also be valuable as a committed test.
| | `e2e:kubernetes:v1alpha1` | Agent Sandbox v1alpha1 compatibility | | ||
| | `e2e:kubernetes:external-driver` | External Kubernetes driver sidecar | | ||
|
|
||
| The Docker, Podman, Kubernetes, and VM runners execute CLI |
There was a problem hiding this comment.
Nit: this paragraph covers all four runners, but it sits under the Kubernetes task-variants table, where Docker, Podman, and VM users are unlikely to look. Consider moving it to a shared e2e section.
Summary
Restore the default Rust E2E coverage when CI exports an empty feature override, and use
e2e-conformance-onlyfor intentional conformance-only runs. Temporarily exclude onlycanonical_main_connect_recovers_its_ssh_transportin the Kubernetes nextest profile while #4445 investigates input-owner cleanup after reconnect. Keep the existing Kubernetes port-forward transport and the test's coverage on other drivers.The empty-feature behavior underlying this CI coverage gap was introduced by #2925, merged on September 1, 2026. That PR changed the runners from
${VAR:-default}to${VAR-default}and made an empty feature list skip Rust E2E tests after conformance. #2260, merged on September 7, then wired the Kubernetes workflow's empty default feature input into that override. Together, these changes caused the default Kubernetes lane to run conformance only while reporting success.Related Issue
No separate implementation issue is required for this localized CI fix, which does not change product behavior or public APIs. The missed coverage is visible in this Kubernetes job.
Temporary Kubernetes-only test waiver: #4445. This PR does not close the investigation.
Changes
e2e-conformance-onlyin the Docker, Podman, Kubernetes, and VM runners and consume it before invoking Cargo.profile.e2e-kubernetes. Keep the other 96 restored default Kubernetes tests enabled, along with recovery coverage on other driver profiles.TESTING.md; update workflow input help. No kind configuration, Service type, or gateway transport changes are included.Testing
mise run pre-commiton the final branch.e2e-kubernetes; a similarly named neighbor and an ordinary lifecycle test remain selected. The recovery name is selected under bothcianddefaultprofiles.git diff --check.mise run ci: the earlier attempt with fresh Go caches hit Rust test linking because system Z3 is unavailable (ld: library 'z3' not found).Checklist