-
Notifications
You must be signed in to change notification settings - Fork 20
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
Switching to web-executor #355
Conversation
By analyzing the blame information on this pull request, we identified @LukasReschke, @DeepDiver1975 and @MorrisJobke to be potential reviewers |
$request = $client->createRequest( | ||
'POST', | ||
// TODO Get endpoint somehow :-/ | ||
'http://localhost/~deo/oc-tmp/index.php/occ/' . $command, |
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.
bäääh ... just another chicken egg issue ....
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.
well - we are only in web executor mode is the updater is executed from the browser.
Within the browser we know the address - right?
@@ -135,6 +134,9 @@ public function ajaxAction() { | |||
$output->setFormatter(new HtmlOutputFormatter($formatter)); | |||
|
|||
$application->setAutoExit(false); | |||
|
|||
$endpoint = preg_replace('/(updater\/|updater\/index.php)$/', '', $this->request->server('REQUEST_URI')); |
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'm honestly bad with regex 🙈
Will this work for setups where index.php is not part of the url?
THX
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.
@DeepDiver1975 yes. It stands for
(updater/OR
updater/index.php)EOL
here we go ... my long standing cry for unit test ... please |
Unit tests please |
@DeepDiver1975 tests added |
@PVince81 @DeepDiver1975 not a WIP anymore, please review |
I tested with the steps above, going from a 9.0.3 tarball + patches to stable9+patches and then to 9.1.0RC1 tarball + patches. Also I observed the access log of Apache and saw that "occ" was used over the web executor. 👍 works |
I suggest we merge this and let @davitol test with tomorrow's stable9/master dailies for easier testing. Would still require patching of 9.0.3. |
[ [ 'headers'=> [ 'HTTP_HOST'=> 'duck', 'SERVER_NAME' => 'jump'] ], 'duck' ], | ||
[ [ 'headers'=> [ 'HTTP_X_FORWARDED_HOST'=>'go', 'HTTP_HOST'=> 'duck', 'SERVER_NAME' => 'jump'] ], 'go' ], | ||
[ [ 'headers'=> [ 'HTTP_X_FORWARDED_HOST'=>'go,', 'HTTP_HOST'=> 'duck', 'SERVER_NAME' => 'jump'] ], 'go' ], | ||
[ [ 'headers'=> [ 'HTTP_X_FORWARDED_HOST'=>'run,forrest,run', 'HTTP_HOST'=> 'duck', 'SERVER_NAME' => 'jump'] ], 'run' ], |
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.
😆
Code looks fine, too. Great job and thanks for the detailed test steps ! |
This will need to be backported to stable9 for 9.0.4. |
@DeepDiver1975 @georgehrke second review please |
Looks good except for the missing explanation of the magic regular expression |
Great! @georgehrke agreed ? |
@PVince81 please hold on for a minute, I'm squashing history |
@PVince81 squashed |
👍 |
QA ticket to redo a full test once the backports are merged: owncloud/QA#277 |
Depends on
master
stable9.1
stable9
Right now web executor is used when update is initiated from Web UI
How to test:
Install OC 9.0.3
Apply [stable9] occ web executor core#25222
Apply Bypass upgrade page when occ controller is requested core#25356
Apply this PR
Specify custom update server url in config.php:
'updater.server.url' => 'https://domain.tld/your_update_server/',
Download master or stable9 daily.
Repack daily applying
(for stable9 daily)
[stable9] occ web executor core#25222 [Stable 9] Bypass upgrade page when occ controller is requested core#25363
(for master daily)
Bypass upgrade page when occ controller is requested core#25356
Put repacked daily to the location your update server points to
Update (from Web UI)