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

Let commands actually set their progress log. #4909

Merged
merged 1 commit into from
Feb 24, 2016

Conversation

rtibbles
Copy link
Member

Summary

Removes the seemingly superfluous foreground flag on UpdatesCommands - as it is set during init, can't be modified post hoc after handle (as suggested in the comment that implemented it).

Reviewer guidance

This is a reversion of a previous change that caused this bug.

Issues addressed

Fixes #4817.

@benjaoming please comment as to why the foreground flag is necessary if I am removing it in this way (and just letting the process_log be None), and I will try to update this to account for that use case, but for now, it just seemed to block the intended use of these UpdatesCommands.

@benjaoming
Copy link
Contributor

@rtibbles Creating the content db template happens from command line, so it's supposed to run in the foreground and not put data in data.sqlite (which isn't even created at that point).

I introduced the --foreground flag especially for make assets, which invokes:

bin/kalite manage retrievecontentpack download en --minimal --foreground --template

I'm not sure why it has affect other things, apologies for that. But please don't remove it, consider this commit:

rtibbles@1b53758#diff-9b6a9dbe0240241084068eb7080204e8

@rtibbles
Copy link
Member Author

OK, will work to make it function as intended - however, from what I can tell, the foreground flag is actually doing nothing, because the options never get parsed anywhere?

So, in the retrievecontentpacks instance, you actually want it not to be foreground, because you want it to run without creating process logs, right? In which case, we should maybe have a 'background' flag that gets invoked instead, and prevents the process logs from being accessed?

@rtibbles
Copy link
Member Author

How about this, @benjaoming? Setting the foreground flag should now prevent it from doing any process_log related malarkey, and each Class now has to call the setup method in handle to make sure the process logs are there if needed.

@benjaoming
Copy link
Contributor

Apparently a blunder in that I set self.foreground to False disregarding my own setup argument, good intention half delivery :) Thanks for fixing it!

benjaoming added a commit that referenced this pull request Feb 24, 2016
Let commands actually set their progress log.
@benjaoming benjaoming merged commit 174c0c3 into learningequality:0.16.x Feb 24, 2016
@benjaoming benjaoming removed the has PR label Feb 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants