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

[upgrade] show timestamp for output during upgrade #17093

Merged
merged 3 commits into from Jul 1, 2015
Merged

Conversation

@MorrisJobke
Copy link
Contributor

MorrisJobke commented Jun 23, 2015

  • added -v option to show timestamp

cc @LukasReschke @DeepDiver1975 @nickvergessen @rullzer @Xenopathic

./occ upgrade -v
ownCloud or one of the apps require upgrade - only a limited number of commands are available
2015-06-23T09:06:15+0000 Turned on maintenance mode
2015-06-23T09:06:15+0000 Checked database schema update
2015-06-23T09:06:15+0000 Checked database schema update for apps
2015-06-23T09:06:15+0000 Updated database
2015-06-23T09:06:15+0000 Updated <files_sharing> to 0.6.6
2015-06-23T09:06:15+0000 Update successful
2015-06-23T09:06:15+0000 Turned off maintenance mode
$ ./occ upgrade
ownCloud or one of the apps require upgrade - only a limited number of commands are available
Turned on maintenance mode
Checked database schema update
Checked database schema update for apps
Updated database
Updated <files_sharing> to 0.6.5
Update successful
Turned off maintenance mode
@MorrisJobke MorrisJobke added this to the 8.2-next milestone Jun 23, 2015
@nickvergessen
nickvergessen reviewed Jun 23, 2015
View changes
core/command/upgrade.php Outdated
*/
protected function writeln(OutputInterface $output, $line) {
$time = new \DateTime();
$t = $time->format(\DateTime::ISO8601);

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 23, 2015

Contributor

Should we make use of logdateformat and logtimezone from system config?

@nickvergessen
nickvergessen reviewed Jun 23, 2015
View changes
core/command/upgrade.php Outdated
protected function writeln(OutputInterface $output, $line) {
$time = new \DateTime();
$t = $time->format(\DateTime::ISO8601);
$output->writeln($t . ' ' . $line);

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 23, 2015

Contributor

You don't take $this->showTimestamp into account?

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Jun 23, 2015

Author Contributor

-.-

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Jun 23, 2015

Author Contributor

I simply forgot to add the code ... it's uncommited here

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 23, 2015

Contributor

dont be sad young fellow, thats why we review

@MorrisJobke MorrisJobke force-pushed the upgrade-timestamp branch to ee1a441 Jun 23, 2015
@MorrisJobke
Copy link
Contributor Author

MorrisJobke commented Jun 23, 2015

$t = '';
if($this->showTimestamp) {
$time = new \DateTime();
$t = $time->format(\DateTime::ISO8601) . ' ';

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 23, 2015

Contributor

Reactivating my comment:
Should we make use of logdateformat and logtimezone from system config?

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 23, 2015

Contributor

pushed

@Xenopathic
Copy link
Member

Xenopathic commented Jun 23, 2015

I can foresee a problem we might have if we try to extend this in the future - if a subclass wants an OutputInterface to write stuff to the console, the best we can do from here is pass on the existing OutputInterface, but that means that output will not contain the timestamp (since that is a wrapper function of this class). Symfony Console supports the concept of formatters though - it might be best to create an OutputFormatter subclass with the format() function overriden, that prepends a timestamp. Then everything is contained in the $output object.

@MorrisJobke
Copy link
Contributor Author

MorrisJobke commented Jun 23, 2015

@Xenopathic Thanks for the pointer :)

@nickvergessen
nickvergessen reviewed Jun 23, 2015
View changes
core/command/upgrade.php Outdated
@@ -110,6 +126,12 @@ protected function execute(InputInterface $input, OutputInterface $output) {
}

if(\OC::checkUpgrade(false)) {
if ($input->getOption('show-timestamp')) {

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 23, 2015

Contributor

I think we could also just use a verbosity level for this or default to true. It does not really hurt to have it. but you can't have it retro-actively

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 23, 2015

Contributor

@MorrisJobke what do you think?

@nickvergessen
nickvergessen reviewed Jun 23, 2015
View changes
lib/private/console/timestampformatter.php Outdated
use Symfony\Component\Console\Formatter\OutputFormatterInterface;
use Symfony\Component\Console\Formatter\OutputFormatterStyleInterface;

class TimestampFormatter extends \Symfony\Component\Console\Formatter\OutputFormatter {

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 23, 2015

Contributor

Could also just implement the interface instead of extending, because we don't use any method of the class anymore now

This comment has been minimized.

Copy link
@Xenopathic

Xenopathic Jun 23, 2015

Member

Agreed - the Wrapper design pattern is a nice one to follow

@Xenopathic
Xenopathic reviewed Jun 23, 2015
View changes
core/command/upgrade.php Outdated
@@ -170,6 +192,9 @@ function () use($output, $updateStepEnabled, $self) {

$this->postUpgradeCheck($input, $output);

if ($input->getOption('show-timestamp')) {
$output->setFormatter($this->originalFormatter);
}

This comment has been minimized.

Copy link
@Xenopathic

Xenopathic Jun 23, 2015

Member

We are returning one line below - this isn't required 😄 (also, one less class member needed)

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 23, 2015

Contributor

right, but the output will continue living, because it was just injected to this method. I know it is not being used anymore atm, but I thought better save then sorry.
Anyway all fine by me

@Xenopathic
Copy link
Member

Xenopathic commented Jun 23, 2015

Nice, looks like clean code to me! (apart from the above comments of course)

@nickvergessen nickvergessen force-pushed the upgrade-timestamp branch to 42ffb39 Jun 23, 2015
@nickvergessen
Copy link
Contributor

nickvergessen commented Jun 23, 2015

@MorrisJobke @Xenopathic should be up to date now, I replaced the additional option with a check for verbosity, I hope that's fine

@MorrisJobke
Copy link
Contributor Author

MorrisJobke commented Jun 23, 2015

@Xenopathic
Copy link
Member

Xenopathic commented Jun 23, 2015

👍

@nickvergessen nickvergessen mentioned this pull request Jun 24, 2015
@nickvergessen nickvergessen force-pushed the upgrade-timestamp branch from 42ffb39 to dba5d5e Jun 24, 2015
@scrutinizer-notifier
Copy link

scrutinizer-notifier commented Jun 24, 2015

A new inspection was created.

@nickvergessen
Copy link
Contributor

nickvergessen commented Jun 24, 2015

rebased due to #17095

@ghost
Copy link

ghost commented Jun 24, 2015

🚀 Test PASSed.🚀
chuck

@MorrisJobke
Copy link
Contributor Author

MorrisJobke commented Jun 24, 2015

As this was nearly completely rewritten by @nickvergessen : I tested this and it works, thanks 👍

@MorrisJobke
Copy link
Contributor Author

MorrisJobke commented Jul 1, 2015

stable8.1 is created -> merge

MorrisJobke added a commit that referenced this pull request Jul 1, 2015
[upgrade] show timestamp for output during upgrade
@MorrisJobke MorrisJobke merged commit 4e44cc4 into master Jul 1, 2015
1 check passed
1 check passed
default Build finished.
Details
@MorrisJobke MorrisJobke deleted the upgrade-timestamp branch Jul 1, 2015
@MorrisJobke MorrisJobke mentioned this pull request Jul 9, 2015
3 of 3 tasks complete
@lock lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.