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

Jump to bottom

core:update displays instructions -after- updating #6317

Closed
khromov opened this issue Sep 27, 2014 · 6 comments
Closed

core:update displays instructions -after- updating #6317

khromov opened this issue Sep 27, 2014 · 6 comments
Assignees
@tsteur
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone
2.9.0

Comments

@khromov
Copy link

khromov commented Sep 27, 2014

When updating using core:update, instead of showing the instructions when running it (the "Database Upgrade Required" message), it shows nothing until the update is completed and then shows the message.

This is kind of confusing, instructions should be shown before because it can take a long time to complete updates, making it look like the script has stalled without any message as it is right now.
@mattab mattab added Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. labels Sep 30, 2014
@mattab
Copy link
Member

mattab commented Sep 30, 2014

Thanks for the suggestion!

@mattab mattab modified the milestones: Piwik 2.8.0, Piwik 2.9.0 Oct 2, 2014
@tsteur
Copy link
Member

tsteur commented Oct 15, 2014

Just had a look at this / wanted to implement this but noticed after a while I'd have to pass the Output instance of the console to the controller or better refactor most parts into a model or something so it is reusable in the command and CLI does not use Controller at all. Is there another script to update Piwik on CLI beside this one?

Alternatively I was wondering if it is needed at all since one can get all the queries via --dry-run.

@mattab
Copy link
Member

mattab commented Oct 17, 2014

It's useful change: otherwise there could be no output on the console which is confusing UX for users.

+1 to refactor the code.

@tsteur
Copy link
Member

tsteur commented Oct 19, 2014

There is always a message (after the update). Maybe it could be enough to only show an initial message?

Otherwise for the person who is going to implement this: It might be a good idea in this case to apply the --dry-run by default (meaning we get rid of this one) and then ask for confirmation whether it shall be executed. Alternatively a --yes parameter (maybe different naming) can be used to confirm this message automatically. The change should be documented / mentioned in blog post

tsteur added a commit that referenced this issue Nov 5, 2014
@tsteur
refs #6317 display instructions before upgrading.
612b250 
We will by default always run a --dry-run before actually upgrading
and then ask the user for confirmation to execute those updates.
Alternatively a user can use a parameter --yes to avoid asking
for confirmation.
@tsteur tsteur mentioned this issue Nov 5, 2014
Merged
@tsteur
Copy link
Member

tsteur commented Nov 5, 2014

See pull request

tsteur added a commit that referenced this issue Nov 5, 2014
@tsteur
Revert "refs #6317 display instructions before upgrading."
add7e34 
This reverts commit 612b250.
tsteur added a commit that referenced this issue Nov 7, 2014
@tsteur
refs #6317 display instructions before upgrading.
aaa2b72 
We will by default always run a --dry-run before actually upgrading
and then ask the user for confirmation to execute those updates.
Alternatively a user can use a parameter --yes to avoid asking
for confirmation.
tsteur added a commit that referenced this issue Nov 7, 2014
@tsteur
Revert "refs #6317 display instructions before upgrading."
66f3e3f 
This reverts commit 612b250.
@tsteur tsteur self-assigned this Nov 7, 2014
@mattab
Copy link
Member

mattab commented Nov 7, 2014

Good work @tsteur

@mattab mattab closed this as completed Nov 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tsteur tsteur

Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Milestone
2.9.0
Development

No branches or pull requests
3 participants
@tsteur @mattab @khromov