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

Improve the error message when running nested inside a non-Toolbx container #1295

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nievesmontero
Copy link
Collaborator

@nievesmontero nievesmontero commented May 4, 2023

Fixes issue #1183

Signed-off-by: Nieves Montero [email protected]

@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/75438452a5fb458ba31b16a11253b4f0

✔️ unit-test SUCCESS in 9m 32s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 57s
✔️ unit-test-restricted SUCCESS in 8m 26s
system-test-fedora-rawhide FAILURE in 20m 30s
system-test-fedora-38 FAILURE in 20m 29s
system-test-fedora-37 FAILURE in 19m 37s
system-test-fedora-36 FAILURE in 19m 42s

@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/e56e05b64ef942c8aeddfed91fe448d6

✔️ unit-test SUCCESS in 9m 44s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 31s
✔️ unit-test-restricted SUCCESS in 8m 20s
system-test-fedora-rawhide FAILURE in 20m 19s
system-test-fedora-38 FAILURE in 20m 18s
system-test-fedora-37 FAILURE in 20m 27s
system-test-fedora-36 FAILURE in 20m 11s

@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/6c859657908e4c7488f85240159e18de

✔️ unit-test SUCCESS in 9m 21s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 20s
✔️ unit-test-restricted SUCCESS in 8m 28s
system-test-fedora-rawhide FAILURE in 20m 01s
system-test-fedora-38 FAILURE in 20m 15s
system-test-fedora-37 FAILURE in 19m 15s
system-test-fedora-36 FAILURE in 19m 10s

@softwarefactory-project-zuul
Copy link

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/02243c22962341c28685ba288cf277e4

✔️ unit-test SUCCESS in 9m 16s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 11s
✔️ unit-test-restricted SUCCESS in 8m 19s
✔️ system-test-fedora-rawhide SUCCESS in 21m 05s
✔️ system-test-fedora-38 SUCCESS in 20m 03s
✔️ system-test-fedora-37 SUCCESS in 19m 12s
✔️ system-test-fedora-36 SUCCESS in 19m 17s

@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/2f82e17a8e084fe89a9ffe964cf125bc

unit-test FAILURE in 9m 18s
unit-test-migration-path-for-coreos-toolbox FAILURE in 2m 44s
unit-test-restricted FAILURE in 8m 19s
system-test-fedora-rawhide FAILURE in 9m 17s
system-test-fedora-38 FAILURE in 9m 13s
system-test-fedora-37 FAILURE in 9m 11s
system-test-fedora-36 FAILURE in 9m 03s

@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/72c6ce0296ea4309a5a2a292a147d3ca

✔️ unit-test SUCCESS in 9m 46s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 43s
✔️ unit-test-restricted SUCCESS in 8m 35s
system-test-fedora-rawhide FAILURE in 20m 14s
system-test-fedora-38 FAILURE in 20m 27s
system-test-fedora-37 FAILURE in 19m 56s
system-test-fedora-36 FAILURE in 20m 02s

@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/e0273448a0a548a3996d9d3785962194

✔️ unit-test SUCCESS in 9m 29s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 30s
✔️ unit-test-restricted SUCCESS in 8m 33s
system-test-fedora-rawhide FAILURE in 20m 03s
system-test-fedora-38 FAILURE in 20m 29s
system-test-fedora-37 FAILURE in 19m 46s
system-test-fedora-36 FAILURE in 19m 42s

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.

Is this a continuation of #1191 ? It would have been better to continue using the old pull request to keep all the discussion in one place.

Anyway, the tests are failing, and the branch has conflicts. Did you check the test logs to understand why they are failing? The tests really need to pass.

@nievesmontero
Copy link
Collaborator Author

nievesmontero commented Jul 14, 2023

Is this a continuation of #1191 ? It would have been better to continue using the old pull request to keep all the discussion in one place.

Anyway, the tests are failing, and the branch has conflicts. Did you check the test logs to understand why they are failing? The tests really need to pass.

I paused this PR and moved to #1301 to first fix the tests. I expect this PR to be fixed once #1301 is fixed.

@debarshiray
Copy link
Member

Is this a continuation of #1191 ? It would have been better to continue using the old pull request to keep all the discussion in one place.
Anyway, the tests are failing, and the branch has conflicts. Did you check the test logs to understand why they are failing? The tests really need to pass.

I paused this PR and moved to #1301 to first fix the tests. I expect this PR to be fixed once #1301 is fixed.

Did the tests get fixed for this pull request? Last time they were failing and they haven't been run lately.

It also looks like this branch conflicts with main. Could you please git rebase ... it?

@debarshiray
Copy link
Member

recheck

@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.
Warning:
Error merging github.com/containers/toolbox for 1295,fcb73d26abb7f0c07379a1a8d47d3de2d4400287

@nievesmontero nievesmontero force-pushed the improve-error-message branch 3 times, most recently from e1f36a9 to 0bf87c5 Compare October 16, 2023 10:25
@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/dbf19b014b674471b3cbda54dd2d1093

unit-test RETRY_LIMIT in 7m 09s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 4m 12s
unit-test-restricted RETRY_LIMIT in 5m 56s
system-test-fedora-rawhide RETRY_LIMIT in 7m 14s
system-test-fedora-39 RETRY_LIMIT in 7m 12s
system-test-fedora-38 RETRY_LIMIT in 7m 12s
system-test-fedora-37 RETRY_LIMIT in 6m 53s

@nievesmontero nievesmontero force-pushed the improve-error-message branch 3 times, most recently from 118ce54 to 47b556a Compare October 16, 2023 11:35
@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/558854fe01d84c308b81e724cf0d859a

unit-test RETRY_LIMIT in 6m 55s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 23s
unit-test-restricted RETRY_LIMIT in 5m 59s
system-test-fedora-rawhide RETRY_LIMIT in 6m 53s
system-test-fedora-39 RETRY_LIMIT in 6m 58s
system-test-fedora-38 RETRY_LIMIT in 6m 53s
system-test-fedora-37 RETRY_LIMIT in 6m 53s

@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/8590f159fe4b4da7af7be1e5808e9de3

✔️ unit-test SUCCESS in 9m 05s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 4m 40s
✔️ unit-test-restricted SUCCESS in 8m 20s
system-test-fedora-rawhide FAILURE in 45m 59s
system-test-fedora-39 FAILURE in 45m 24s
✔️ system-test-fedora-38 SUCCESS in 34m 02s
✔️ system-test-fedora-37 SUCCESS in 39m 11s

Copy link
Member

@HarryMichal HarryMichal left a comment

Choose a reason for hiding this comment

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

You seem to have left the check commented out.

Before this goes in having a test would be nice. Some along the lines of running podman run -it <some-available-image-in-the-test-suit> toolbox? Do we need the test to be done per command? Probably not.

src/cmd/root.go Outdated
Comment on lines 147 to 150
/*if utils.IsInsideContainer() && !utils.IsInsideToolboxContainer() {
return createErrorNonToolboxContainer()
}*/

Copy link
Member

Choose a reason for hiding this comment

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

This probably is not meant to be commented out.

Suggested change
/*if utils.IsInsideContainer() && !utils.IsInsideToolboxContainer() {
return createErrorNonToolboxContainer()
}*/
if utils.IsInsideContainer() && !utils.IsInsideToolboxContainer() {
return createErrorNonToolboxContainer()
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not, however, I was testing this because if I remove the comment, all the tests fail on the CI.

Copy link
Member

Choose a reason for hiding this comment

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

The problem in the CI is caused by the fact that when you run toolbox init-container inside the container, the /run/.toolboxenv file does not exist, because it is created by the command. So, it is a classic chicken-and-egg problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any idea on how to face this problem @HarryMichal ?

Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/c52c6f52e84f4cd68354676f2e801acb

✔️ unit-test SUCCESS in 9m 13s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 37s
✔️ unit-test-restricted SUCCESS in 9m 17s
system-test-fedora-rawhide FAILURE in 27m 13s
system-test-fedora-39 FAILURE in 28m 57s
system-test-fedora-38 FAILURE in 28m 00s
system-test-fedora-37 FAILURE in 28m 30s

Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/83dd5a44ba224679ab96f7567d18f055

✔️ unit-test SUCCESS in 8m 38s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 9m 17s
✔️ unit-test-restricted SUCCESS in 8m 02s
system-test-fedora-rawhide FAILURE in 28m 18s
system-test-fedora-39 FAILURE in 28m 18s
system-test-fedora-38 FAILURE in 28m 13s
system-test-fedora-37 FAILURE in 27m 55s

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.

Ping. What's the status of this pull request?

@nievesmontero
Copy link
Collaborator Author

nievesmontero commented Dec 11, 2023

Ping. What's the status of this pull request?

I put it aside while working on the other PRs. I am stuck because of the problem mentioned here #1295 (comment) by @HarryMichal
The changes I made to add the error make the tests fail on the CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants