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.

Sign up for GitHub

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

Jump to bottom

Adding a test for $PWD in the container #370

Merged
merged 1 commit into from
Jun 23, 2020
Merged

Adding a test for $PWD in the container #370

merged 1 commit into from
Jun 23, 2020

Conversation

bureado
Copy link
Contributor

@bureado bureado commented Feb 1, 2020

This is a suggestion to address #369, input and changes welcome.

I didn't want to count on coreutils so ran test inside sh, but perhaps that's fine.

@softwarefactory-project-zuul
Copy link

Build succeeded.

@bureado
Copy link
Contributor Author

bureado commented Feb 1, 2020

Also FYI, I'm not sure if cgwalters/coretoolbox is affected because I couldn't get past its attempt to bind itself, but a cursory look at the code didn't raise any flags. Just mentioning because of #319

@HarryMichal
Copy link
Member

Hi @bureado! I'm currently looking at your PR and I'm wondering that instead of just showing the error message Toolbox could try to mount the missing path. The reason why I think this could be viable is that most paths on the host are inside of the container (they are bind-mounted in /run/host).

Also now that I'm thinking about it. This seems to be kinda similar to #100 and #348. Both are a bit different but in both cases, one of the steps to solve them is to add user-specifiable bind mounts.

But for now, the easiest solution is to use your approach. I'll add it to the Go version of Toolbox.

HarryMichal pushed a commit to HarryMichal/toolbox that referenced this pull request Mar 20, 2020
@bureado @HarryMichal
Add a test if $PWD is available in the container
c4ac9c0 
In some cases the current directory is not available in a container.
Until now if such scenario happened, then Toolbox would fail silently.
The introduced test uses the 'test' utility to check if the current
working directory is available.

See: containers#370
Closes: containers#369
HarryMichal pushed a commit to HarryMichal/toolbox that referenced this pull request Mar 20, 2020
@bureado @HarryMichal
Add a test if $PWD is available in the container
5eff6f4 
In some cases the current directory is not available in a container.
Until now if such scenario happened, then Toolbox would fail silently.
The introduced test uses the 'test' utility to check if the current
working directory is available.

See: containers#370
Closes: containers#369
HarryMichal pushed a commit to HarryMichal/toolbox that referenced this pull request Mar 20, 2020
@bureado @HarryMichal
Add a test if $PWD is available in the container
f558698 
In some cases the current directory is not available in a container.
Until now if such scenario happened, then Toolbox would fail silently.
The introduced test uses the 'test' utility to check if the current
working directory is available.

See: containers#370
Closes: containers#369
HarryMichal pushed a commit to HarryMichal/toolbox that referenced this pull request Apr 8, 2020
@bureado @HarryMichal
Add a test if $PWD is available in the container
43710b5 
In some cases the current directory is not available in a container.
Until now if such scenario happened, then Toolbox would fail silently.
The introduced test uses the 'test' utility to check if the current
working directory is available.

See: containers#370
Closes: containers#369
@abitrolly
Copy link

@HarryMichal why not to commit that to master until Go rewrite is ready?

debarshiray
debarshiray reviewed Jun 23, 2020
View reviewed changes
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.

Thanks for the nice patch! I like the general approach.

toolbox Outdated Show resolved Hide resolved
@debarshiray debarshiray force-pushed the check-pwd-in-container branch from 5ea2d2e to 5d9ab94 Compare June 23, 2020 17:20
@bureado @debarshiray
cmd/run, sh: Show an error if $PWD is missing inside the container
89ff98d 
It might happen that the current working directory in the present
environment is missing inside the container that's going to be used.
So far, Toolbox would fail silently, which wasn't very obvious.

Some changes by Debarshi Ray and Harry Míchal.

containers#369
@debarshiray debarshiray force-pushed the check-pwd-in-container branch from 5d9ab94 to 89ff98d Compare June 23, 2020 17:27
@debarshiray debarshiray merged commit 89ff98d into containers:master Jun 23, 2020
@debarshiray
Copy link
Member

Thank you for your contribution, and apologies for the delay in getting to it!

HarryMichal added a commit to HarryMichal/toolbox that referenced this pull request Sep 23, 2020
@HarryMichal
cmd/run: Use home folder when $PWD is not in toolbox
ef8299d 
Since v0.0.91[0] Toolbox throws an error if $PWD is not available in a
toolbox. While this fixes the problem with 'toolbox enter/run' silently
failing to enter/exec in a container, it still requires an action to be
made by the user. I believe it is better to handle such situations more
gracefully by falling back to entering the user's home folder + printing
a warning about doing so.

[0] containers#370
@HarryMichal HarryMichal mentioned this pull request Sep 23, 2020
Merged
HarryMichal added a commit to HarryMichal/toolbox that referenced this pull request May 24, 2021
@HarryMichal
cmd/run: Use home folder when $PWD is not in toolbox
25c81f3 
Since v0.0.91[0] Toolbox throws an error if $PWD is not available in a
toolbox. While this fixes the problem with 'toolbox enter/run' silently
failing to enter/exec in a container, it still requires an action to be
made by the user. I believe it is better to handle such situations more
gracefully by falling back to entering the user's home folder + printing
a warning about doing so.

[0] containers#370
HarryMichal added a commit to HarryMichal/toolbox that referenced this pull request May 24, 2021
@HarryMichal
cmd/run: Use home folder when $PWD is not in toolbox
18a38b9 
Since v0.0.91[0] Toolbox throws an error if $PWD is not available in a
toolbox. While this fixes the problem with 'toolbox enter/run' silently
failing to enter/exec in a container, it still requires an action to be
made by the user. I believe it is better to handle such situations more
gracefully by falling back to entering the user's home folder + printing
a warning about doing so.

[0] containers#370
HarryMichal added a commit that referenced this pull request May 24, 2021
@HarryMichal
cmd/run: Use home folder when $PWD is not in toolbox
3db59ab 
Since v0.0.91[0] Toolbox throws an error if $PWD is not available in a
toolbox. While this fixes the problem with 'toolbox enter/run' silently
failing to enter/exec in a container, it still requires an action to be
made by the user. I believe it is better to handle such situations more
gracefully by falling back to entering the user's home folder + printing
a warning about doing so.

[0] #370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@debarshiray debarshiray debarshiray left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bureado @HarryMichal @abitrolly @debarshiray