Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Add support for windows shells and bash #1749

Merged
merged 1 commit into from
Jan 30, 2021

Conversation

GuessWhoSamFoo
Copy link
Contributor

@GuessWhoSamFoo GuessWhoSamFoo commented Dec 18, 2020

What this PR does / why we need it:
Iterate through common shells to give terminal instances a better chance of running.

Which issue(s) this PR fixes

Signed-off-by: GuessWhoSamFoo [email protected]

Copy link
Contributor

@wwitzel3 wwitzel3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see this being able to explicitly set it when the shell is known. For example, in the case of an ephemeral debugging image, we know what shell will be available.

@wwitzel3
Copy link
Contributor

I'm going to think about how and what about this we might want to be testing.

@GuessWhoSamFoo
Copy link
Contributor Author

GuessWhoSamFoo commented Dec 18, 2020

Current plan is to reduce the space in half by finding out if it is a windows container based on the presents of a runtimeClass, tolerations, or a nodeSelector in the pod spec (checking volume mounts for a path seems redundant).

There doesn't seem to be a standard way to tell the default shell in a container otherwise. In other words, octant still has to guess if a given linux container uses sh, bash, something else, or none of them.

@wwitzel3 wwitzel3 added this to In progress in 0.17 via automation Jan 14, 2021
@GuessWhoSamFoo GuessWhoSamFoo moved this from In progress to Sprint in 0.17 Jan 21, 2021
@GuessWhoSamFoo GuessWhoSamFoo force-pushed the issue-1533 branch 2 times, most recently from 765afb6 to f4cde01 Compare January 22, 2021 02:24
@GuessWhoSamFoo
Copy link
Contributor Author

Rebased and updated to check for windows labels before attempting any of the windows shells. Per https://kubernetes.io/docs/setup/production-environment/windows/intro-windows-in-kubernetes/ , Windows containers have to be scheduled on Windows nodes so it is same to assume a pod schedule for a windows node must run a windows container.

@GuessWhoSamFoo GuessWhoSamFoo moved this from Sprint to In progress in 0.17 Jan 25, 2021
0.17 automation moved this from In progress to Reviewer approved Jan 30, 2021
@wwitzel3 wwitzel3 merged commit 7aef5df into vmware-archive:master Jan 30, 2021
0.17 automation moved this from Reviewer approved to Done Jan 30, 2021
@GuessWhoSamFoo GuessWhoSamFoo deleted the issue-1533 branch March 30, 2021 06:13
@lloydchang
Copy link

lloydchang commented May 1, 2021

To Whom It May Concern:

Consider upgrading to Octant v0.19.0 if you're using Octant v0.17.0 and v0.18.0.

Why: Feature Change default shell to bash #1533's initial implementation, Add support for windows shells and bash #1749 released in v0.17.0 and v0.18.0, has a bug of:

OCI runtime exec failed: ...
(process exited)

with operating systems without bash, e.g. Alpine Linux with sh only.

A bug fix increase timeout for remote cmd stream setup #2258 released in v0.19.0, fixed a similar bug No option to choose the interpreter on the terminal #2164.

After the bug fix, operating systems without bash, such as Alpine Linux, can start sh successfully.

Thank you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
0.17
  
Done
Development

Successfully merging this pull request may close these issues.

Change default shell to bash
3 participants