@diosmosis opened this issue on April 20th 2015

Includes following changes: - Moved 'run as superuser' hacks in CronArchive to CliMulti. Instead of getting a token auth and appending it to the URL before using CliMulti, there's a method CliMulti::runAsSuperUser that will do this work transparently. The token auth is only used when doing web requests; RequestCommand now has a --superuser option. - Moved web archiving to a superuser only API method in CoreAdminHome. This simplifies the archive.php script greatly, since we can rely on Piwik's infrastructure to setup & load plugins, and to authenticate the superuser. - Added php-cgi specializations to Console (from an old incorrectly merged PR) so archive.php does not have to be aware of php-cgi. - Removed the need to supply a host in the URL to CliMulti, which means the URL is no longer needed for CronArchive either. - Removed the no longer used method CronArchive::runScheduledTasksInTrackerMode. - Removed unnecessary setup code in CronArchive. FrontController init, for example, is not needed when going through Console, and no longer needed for web archiving since we use an API method. - Removed the test skipping from ArchiveCronTest/ArchiveWebTest. If they randomly fail in the future, I will fix the random failure. - Removed superuser access change and removed the call to Tracker::initCorePiwikInTrackerMode() in Piwik\Tracker\ScheduledTasksRunner.

@mnapoli commented on May 10th 2015

more code removal than addition :heart_eyes:

Love most of the changes (the rest I'm not familiar enough to appreciate it), really awesome stuff.

Also this big pull request could benefit from some rebasing maybe (lots of merge commits, "fix test", etc.). It's not critical but I believe we should try to start doing it a little more now that we do more branches, it would help with diffs and later when doing git history/blame)

@diosmosis commented on May 10th 2015

Love most of the changes (the rest I'm not familiar enough to appreciate it), really awesome stuff.

It's gonna get better when I get back to DI! ;)

@diosmosis commented on May 10th 2015

Also this big pull request could benefit from some rebasing maybe (lots of merge commits, "fix test", etc.).

I dislike rebasing w/ larger changes like this. Instead of dealing w/ one set of conflicts that occur w/ a merge, I have to deal w/ one set of conflicts per commit I made. Then when I push the new branch, since the commit hashes change, github will list the commits twice, instead of just displaying the new ones. If we decide as a team to only do rebases, then I'm fine w/ doing it only, but given the choice I'd prefer to merge w/ master when it's more convenient.

@mnapoli commented on May 10th 2015

Instead of dealing w/ one set of conflicts that occur w/ a merge, I have to deal w/ one set of conflicts per commit I made

That happens rarely (at least for me, very rarely). It may happen for submodules or weird stuff like that, that's why I never mess with submodules in branches. The plus side is that the conflict happens at the correct point in the history so it's easier to fix it (because you are fixing it in the commit that created the conflict, so the changes are much smaller and "targeted").

Then when I push the new branch, since the commit hashes change, github will list the commits twice, instead of just displaying the new ones.

Are you sure you refreshed the page after that? I've never seen this problem, except when I don't refresh the page. Also make sure you push with --force.

I've been rebasing in almost all my pull requests since the beginning (lately with more success, I'm really getting the hang of it). I usually hack away on my branches and then rebase against master to resolve any conflicts and re-order commits, squash commits together, etc… I'm trying to do it more and more to keep a history where 1 commit = 1 change. Rebasing (i.e. not only for solving conflicts, but also for merging commits together and reordering commits) is absolutely awesome for that (and PhpStorm makes it so easy).

Anyway I don't care much, I just think the more we will play with it the more it might benefit to the project, at least that's what I'm experiencing for now.

(edit: I also use amend when committing, pretty useful to add to the previous commit without going through a rebase)

This issue was closed on May 10th 2015
Powered by GitHub Issue Mirror