Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying user shell at toolbox creation #445

Closed
wants to merge 1 commit into from

Conversation

yann-soubeyrand
Copy link

@yann-soubeyrand yann-soubeyrand commented May 15, 2020

When creating a toolbox the user shell can be specified and this shell will be
used when entering the toolbox.

Signed-off-by: Yann Soubeyrand [email protected]

Closes #413

@yann-soubeyrand yann-soubeyrand changed the title Allow specifying user shell at toolbox creation (#413) Allow specifying user shell at toolbox creation May 15, 2020
@yann-soubeyrand yann-soubeyrand changed the title Allow specifying user shell at toolbox creation Allow specifying user shell at toolbox creation (Closes #413) May 15, 2020
@yann-soubeyrand yann-soubeyrand changed the title Allow specifying user shell at toolbox creation (Closes #413) Allow specifying user shell at toolbox creation May 15, 2020
@softwarefactory-project-zuul
Copy link

Build failed.

@yann-soubeyrand
Copy link
Author

Build failed.

* [shellcheck ](https://softwarefactory-project.io/zuul/t/local/build/15af452961464632940b3d9affe9a94d) : SUCCESS in 1m 13s

* [test-with-podman-stable-f30 ](https://softwarefactory-project.io/zuul/t/local/build/fbb58d02449848c7abb0b2a3f2af221d) : FAILURE in 2m 49s

* [test-with-podman-stable-f31 ](https://softwarefactory-project.io/zuul/t/local/build/c00aefea732d42d8a8a5f0ee78a8fada) : FAILURE in 2m 28s

* [test-with-podman-rawhide ](https://softwarefactory-project.io/zuul/t/local/build/d3f373a87e984eac93021e0be87472b7) : FAILURE in 2m 42s

Is it some kind of normal error or did I break something? It seems that meson cannot find the go compiler.

@yann-soubeyrand
Copy link
Author

Hi,
@HarryMichal, @debarshiray, could you have a look at this PR and tell me if I should rework it please?

@softwarefactory-project-zuul
Copy link

Build failed.

@softwarefactory-project-zuul
Copy link

Build failed.

@HarryMichal
Copy link
Member

@yann-soubeyrand, thank you for the patch! I'm currently focusing on v0.1.0 that does not add any new features. When we get that out, I'll review this in-depth.

@HarryMichal HarryMichal added 3. Enhancement Improvement to an existing feature 6. Major Change May cause breakage labels Jul 15, 2020
@HarryMichal HarryMichal added this to the Release 0.2.0 milestone Jul 15, 2020
@softwarefactory-project-zuul
Copy link

Build failed.

@debarshiray
Copy link
Member

What's the use-case for this?

Toolbox containers are so-called pet containers, and meant to be somewhat long-lived interactive command-line environments. Hence, I'd expect users to configure their default shell in the usual way.

@yann-soubeyrand
Copy link
Author

Hello @debarshiray,

Indeed, I configure my default shell (zsh) in my toolbox container. However, I keep the system default shell on the host (bash on Fedora Silverblue). This is for several reasons: I try to avoid installing extra packages on my Silverblue host, and having a different shell on the host and inside my toolbox allows me to quickly differentiate a host shell and a toolbox shell.

As I understood, you validated the idea (#413 (comment)), that's why I've worked on it ;-)

@debarshiray
Copy link
Member

However, I keep the system default shell on the host (bash on
Fedora Silverblue). This is for several reasons: I try to avoid
installing extra packages on my Silverblue host, and having a
different shell on the host and inside my toolbox allows me to
quickly differentiate a host shell and a toolbox shell.

Indeed. Not having to install extra packages on a Silverblue host is a very good reason.

As I understood, you validated the idea (#413 (comment)), that's why I've worked on it ;-)

Oops, my mistake. I was quickly going through the open PRs and forgot about the earlier issue. Sorry about that.

@yann-soubeyrand
Copy link
Author

As I understood, you validated the idea (#413 (comment)), that's why I've worked on it ;-)

Oops, my mistake. I was quickly going through the open PRs and forgot about the earlier issue. Sorry about that.

No problem ;-)

I've just rebased my PR so that it applies cleanly.

@softwarefactory-project-zuul
Copy link

Build succeeded.

Base automatically changed from master to main March 25, 2021 22:25
@yann-soubeyrand
Copy link
Author

Hello @HarryMichal @debarshiray, is there something blocking this PR which I can act on?

@softwarefactory-project-zuul
Copy link

Build succeeded.

When creating a toolbox the user shell can be specified and this shell will be
used when entering the toolbox.

Signed-off-by: Yann Soubeyrand <[email protected]>
@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@debarshiray
Copy link
Member

I suppose this needs to rebased after #813

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!

src/cmd/create.go Show resolved Hide resolved
src/cmd/run.go Show resolved Hide resolved
@debarshiray
Copy link
Member

I am going to close this pull request because it would need a complete re-write. See my comments above.

However, we still want the feature, so please feel free to submit a new pull request.

@debarshiray
Copy link
Member

I am going to close this pull request because it would need a complete re-write. See my comments above.

However, we still want the feature, so please feel free to submit a new pull request.

@yann-soubeyrand has a work in progress at #1037

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. Enhancement Improvement to an existing feature 6. Major Change May cause breakage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: specify shell at toolbox creation time
3 participants