-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
@@ -304,4 +338,17 @@ private function requestUrls(array $piwikUrls) | |||
return $results; | |||
} | |||
|
|||
public static function getSuperUserTokenAuths() |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
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) |
It's gonna get better when I get back to DI! ;) |
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. |
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").
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 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 |
…iMulti), remove test skipping in ArchiveWebTest & ArchiveCronTest.
…in UrlHelper::getArrayFromQueryString).
…set in Fixture.php).
…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 to CliMulti.
… CliMulti (no longer needed by CronArchive).
…heduledTasksRunner.
… no longer relevant.
…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.
c41a979
to
c5b9b89
Compare
Refactoring CronArchive setup + use for less technical debt
Includes following changes: