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

allow syncing single users, add verbose and very verbose output #28212

Closed
wants to merge 5 commits into from

Conversation

butonic
Copy link
Member

@butonic butonic commented Jun 26, 2017

fixes #27707

@tomneedham
Copy link
Member

We should get this in 10.0.3 to help big instances sync specific users + aid scripting

@jvillafanez
Copy link
Member

I'll have to review this once this is ready. There are some code pieces misplaced

@PVince81 PVince81 added the p2-high Escalation, on top of current planning, release blocker label Aug 7, 2017
@PVince81
Copy link
Contributor

PVince81 commented Aug 7, 2017

moving to "triage" for later rescheduling

@PVince81
Copy link
Contributor

unfinished, moving to "planned"

@PVince81 PVince81 modified the milestones: planned, development Aug 10, 2017
@butonic butonic force-pushed the sycn-single-user branch 2 times, most recently from 2cf5e19 to 3743fa6 Compare September 5, 2017 21:06
@PVince81
Copy link
Contributor

PVince81 commented Sep 6, 2017

please post the test plan that you used when testing this

*/
public function run(\Closure $callback) {
public function run(OutputInterface $output, $maxUsers = 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the place to use symfony's OutputInterface nor ProgressBar. This class shouldn't know how to write in the console, and it should be limited to call the callback for each user.
The callback approach is a better choice in this case.

use Symfony\Component\Console\Formatter\OutputFormatterInterface;
use Symfony\Component\Console\Output\OutputInterface;

class OutputAdapter implements OutputInterface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need someone to convince me this is a good idea.

I think @DeepDiver1975 already created a ConsoleLogger somewhere to provide a log interface connected to a symfony console output. It might be better to enrich that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I got confused with https://github.com/owncloud/core/blob/master/lib/private/Migration/ConsoleOutput.php which is similar to what I had in mind...

I'll recheck this

@jvillafanez
Copy link
Member

My main concern is that we should bring back the callback approach to the sync service. The sync service should just focus on syncing, not writing; the callback will take care of writing whatever is needed, or move the progress bar, or send an email to the synced user.

If you don't like callbacks, you can create a custom interface for the sync service, so the sync service can call whatever method of the interface.

use \OCP\IUser;
public interface ISyncServiceCallback {
    public function callMeWith(IUser $user);
}

Whatever happens there isn't sync service's bussines.

@butonic
Copy link
Member Author

butonic commented Sep 7, 2017

@jvillafanez I agree that having the OutputInterface here looks messy. The OutputAdapter even more. But using the IOutput interface to log OR write verbose messages to the console does not work either, because then I have no way of finding out the verbosity level.

Let us take a step back and see if we can agree on the overall output design:

  1. At any point in the code you might want to tell the admin what is going on.
    a. For web requests the output should land in the log file
    b. For CLI requests the output should land in the console (as well) so the admin knows what is happening.
  2. The admin should be able to tune the verboseness as he likes.
  3. Some commands, eg repair steps should always use debug level logging in case something goes wrong.
  4. On an error we should always log the trace.

Can we agree on this?

The SyncService is a nice example. It is executed when a user is provisioned during a web migration (and the setupAccount should bu used by the User Manager to provision new users, instead of duplicating code). And it is executed via CLI command.

So, I agree that the SyncService should not know about writing to the console. But as a developer we still need to be able to determine how verbose we should be. The Logger uses the loglevel from the config to determine that. The CLI can be configured via the -vvv param. The OutputInterface knows how verbose messages should be, which solves the problem nicely. It also allows writing code that does not compute expensive log messages that never make it into the log because it is not set to debug, without having to get an instance of the Config to read the log level.

It would be my wish to use the OutputInterface instead of the Logger everywhere, because it is much more powerful. The classes don't need to know whether the output goes to the web, the log or the CLI, but it should be able to easily determine how verbose it should be and log accordingly.

Using a callback interface makes logging easier, but does not make showing messages to the admin any easier.

So ... I have not yet thought much about this because I think it would require rewiring the whole logging stuff. Maybe we can implement an ILogger that uses an OutputStream to log, add a getOutputStream() to the server container and then subsequently use it instead of getLogger(). The OutputStream could write to the log as well as the CLI.

Or we extend the ILogger with getVerbosity methods ... but then we still need to make it write to the console.

Meh ... @PVince81 @tomneedham @jvillafanez thoughts?

@PVince81 PVince81 added this to the planned milestone Nov 3, 2017
@PVince81 PVince81 modified the milestones: planned, development Nov 22, 2017
@DeepDiver1975
Copy link
Member

@ownclouders rebase

@ownclouders
Copy link
Contributor

Hey! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost!

@ownclouders
Copy link
Contributor

Automated rebase was successful!

@felixboehm felixboehm added p1-urgent Critical issue, need to consider hotfix with just that issue and removed p2-high Escalation, on top of current planning, release blocker labels Dec 14, 2017
@tomneedham tomneedham closed this Dec 22, 2017
@tomneedham
Copy link
Member

Closing to stop people using this. We will reimplement this after #29669 since things brings further code de-duplication and refactoring which is necessary.

@PVince81 PVince81 deleted the sycn-single-user branch September 27, 2018 13:36
@lock lock bot locked as resolved and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3 - To Review p1-urgent Critical issue, need to consider hotfix with just that issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add a uid parameter to the user:sync command
7 participants