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

Refactoring CronArchive setup + use for less technical debt #7707

Merged
merged 21 commits into from May 10, 2015

Conversation

diosmosis
Copy link
Member

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.

@diosmosis diosmosis added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Apr 20, 2015
@diosmosis diosmosis added this to the Piwik 2.14.0 milestone Apr 20, 2015
@diosmosis diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels May 8, 2015
@@ -304,4 +338,17 @@ private function requestUrls(array $piwikUrls)
return $results;
}

public static function getSuperUserTokenAuths()
Copy link
Contributor

Choose a reason for hiding this comment

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

is it necessary that it is public? (in the diff it seems to be called only from this class)

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess not anymore, will make it private.

@mnapoli
Copy link
Contributor

mnapoli commented May 10, 2015

more code removal than addition 😍

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

mnapoli commented May 10, 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)

diosmosis added 16 commits May 9, 2015 22:40
…iMulti), remove test skipping in ArchiveWebTest & ArchiveCronTest.
…d there & add TODO for deprecating misc/cron/archive.php.
… archive.php script and re-add php-cgi support to Console, which did not get merged in the past for some reason.
… CliMulti (no longer needed by CronArchive).
diosmosis added 5 commits May 9, 2015 22:49
…hive.php is still required, so we'll just test that.
… are run as superuser in CronArchive. Remove TODO, archive.php script works w/ php-cgi.
…here, and include bootstrap.php in archive.php cron script only when creating a Console instance.
@diosmosis diosmosis force-pushed the cron_archive_setup_refactor branch from c41a979 to c5b9b89 Compare May 10, 2015 05:55
mnapoli added a commit that referenced this pull request May 10, 2015
Refactoring CronArchive setup + use for less technical debt
@mnapoli mnapoli merged commit 7d62b91 into master May 10, 2015
@mnapoli mnapoli deleted the cron_archive_setup_refactor branch May 10, 2015 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants