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

Catch installation errors and display them. #952

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

almet
Copy link
Contributor

@almet almet commented Oct 14, 2024

This catches the errors that can happen during installation, and during some other phases, and display them to the end user in the interface.

Fixes #193

@almet almet force-pushed the 193-container-installation-failure branch 12 times, most recently from d2fd81c to 585f6f3 Compare October 15, 2024 14:51
@almet
Copy link
Contributor Author

almet commented Oct 15, 2024

On MacOS, the tests are currently failing. I managed to reproduce on our mac mini. For a reason I still need to understand, the following command hangs indefinitely.

usr/local/bin/docker image list --format "{{.ID}}" dangerzone.rocks/dangerzone

And as a result the GUI continues showing that it's "installing" the image.

@almet
Copy link
Contributor Author

almet commented Oct 15, 2024

Tests are now passing (without doing any changes). I'm not sure if it was due to an error with the runners? 🤔
I re-triggered them and they seem to be green each time now.

@apyrgio I believe this is ready for review.

@apyrgio
Copy link
Contributor

apyrgio commented Oct 16, 2024

Tested the PR on a Windows environment. Aside from a stray exception, everything else seems to work nicely. Nice job!

@apyrgio
Copy link
Contributor

apyrgio commented Oct 16, 2024

Not much else to comment on, I think it's good to merge.

@almet
Copy link
Contributor Author

almet commented Oct 17, 2024

Tests were intermittently failing on MacOS and Windows, due to the way we tested threads, which was a bit brittle. I changed the approach to flatten threads during the tests (replacing .start() with .run()) to ensure the logic is tested properly.

If that's okay for you @apyrgio, and that tests are green on all targets, I'll wait until #748 is merged, rebase and we should be good.

@almet almet force-pushed the 193-container-installation-failure branch 2 times, most recently from 4caad39 to 8ef0a76 Compare October 17, 2024 13:48
@almet almet force-pushed the 193-container-installation-failure branch from 8ef0a76 to a95b612 Compare October 17, 2024 14:21
@almet almet merged commit a95b612 into main Oct 17, 2024
90 checks passed
@almet almet deleted the 193-container-installation-failure branch October 17, 2024 14:56
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.

Handle container install failure
2 participants