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

importcontent command should allow failure on import errors #9258

Closed
dbnicholson opened this issue Apr 8, 2022 · 3 comments · Fixed by #9259
Closed

importcontent command should allow failure on import errors #9258

dbnicholson opened this issue Apr 8, 2022 · 3 comments · Fixed by #9259

Comments

@dbnicholson
Copy link
Contributor

Observed behavior

When running the importcontent management command, it succeeds with a warning when files failed to be imported.

Errors and logs

WARNING  32 files are skipped, because errors occurred during the import.

Expected behavior

importcontent fails when it can't import all files.

User-facing consequences

If you're trying to provision a fully populated device, then you'll have to dig through the logs to find out if it really succeeded or not.

Steps to reproduce

python -m kolibri manage importchannel network 2091ca47ff544c96b4ae02b3a92346e1
python -m kolibri manage importcontent network --baseurl https://example.com 2091ca47ff544c96b4ae02b3a92346e1
echo $?

The last step would print 0 while I'd expect it to have some non-0 failure code.

Context

This is the develop branch as of 6f7f0b8 on Linux (EndlessOS). Where this really affects us is in our image builder. There we're installing Kolibri via the org.learningequality.Kolibri flatpak app in flathub. Currently this uses kolibri-installer-gnome 2.0, which in turn includes Kolibri 0.14.7 (AFAICT).

We want to pre-populate our OS images with Kolibri content. If the content isn't reliably downloaded, then we might ship images that are missing channel objects.

@rtibbles
Copy link
Member

rtibbles commented Apr 8, 2022

This is deliberate, as much of the download is taking place in a places with unreliable connectivity - and causing the entire import to fail because of a few files is undesirable.

A fix that would encompass the use case you are describing would be to add a --fail-fast or --error-on-fail flag to the command that could cause the command to fail when a file download fails.

@dbnicholson
Copy link
Contributor Author

Sorry, I should have phrased that differently. I did find the old commits that made it that way. I'll make a PR with a flag.

@dbnicholson dbnicholson changed the title importcontent command should fail on import errors importcontent command should allow failure on import errors Apr 8, 2022
dbnicholson added a commit to dbnicholson/kolibri that referenced this issue Apr 8, 2022
Normally importcontent carries on when it can't import a file and prints
a warning on the number of skipped files. That makes it unreliable when
trying to ensure a fully provisioned device. Add the --error-on-fail
option to make import errors fatal.

Fixes: learningequality#9258
dbnicholson added a commit to dbnicholson/kolibri that referenced this issue Apr 8, 2022
Normally importcontent carries on when it can't import a file and prints
a warning on the number of skipped files. That makes it unreliable when
trying to ensure a fully provisioned device. Add the --error-on-fail
option to make import errors fatal.

Fixes: learningequality#9258
dbnicholson added a commit to dbnicholson/kolibri that referenced this issue Apr 9, 2022
Normally importcontent carries on when it can't import a file and prints
a warning on the number of skipped files. That makes it unreliable when
trying to ensure a fully provisioned device. Add the --fail-on-error
option to make import errors fatal.

Fixes: learningequality#9258
@rtibbles
Copy link
Member

Fixed in #9259

AtKristijan pushed a commit to AtKristijan/kolibri that referenced this issue Apr 19, 2022
Normally importcontent carries on when it can't import a file and prints
a warning on the number of skipped files. That makes it unreliable when
trying to ensure a fully provisioned device. Add the --fail-on-error
option to make import errors fatal.

Fixes: learningequality#9258
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 a pull request may close this issue.

2 participants