Skip to content

Conversation

@MorrisJobke
Copy link
Member

  • show common HTTP phrases instead of cryptic status codes (404, 401, 503, ...)
  • add a lot of log output
  • add "requestID" to be able to track logs of a single request and log current version

cc @LukasReschke @nickvergessen

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Sep 21, 2016

How to test:

  1. setup a local update server that serves an update URL
  2. set following two options in config.php accordingly, like:
    • 'updater.server.url' => 'http://192.168.0.25/updater_server/',
    • 'updater.release.channel' => 'daily',
  3. place the index.php of this PR into updater/index.php
  4. go to admin page, scroll to the updater section and click the "Open updater" button

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Sep 21, 2016

Also closing the browser should work fine. The only disadvantage then is that you need to generate a new secret and write it to the config.php, because that was done by the "Open updater" button logic and was newer shown to the user.

The updater then resumes at the step that was finished before.

@MorrisJobke
Copy link
Member Author

The last commit addresses nextcloud/server#660 😉


/** @var array $CONFIG */
$configFileName = __DIR__ . '/../config/config.php';
require $configFileName;
Copy link
Member

Choose a reason for hiding this comment

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

Add a check if the config file really exists, doesn't have to be the case with NEXTCLOUD_CONFIG_DIR.

$charactersLength = strlen($characters);
$randomString = '';
for ($i = 0; $i < 10; $i++) {
$randomString .= $characters[rand(0, $charactersLength - 1)];
Copy link
Member

Choose a reason for hiding this comment

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

I don't want people complaining about that. Can we just use http://php.net/manual/en/function.random-int.php?

Also you might want to add $_SERVER['UNIQUE_ID'] which is used by http://httpd.apache.org/docs/current/mod/mod_unique_id.html

Copy link
Member Author

Choose a reason for hiding this comment

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

It's for making the log more readable.

index.php Outdated
} else {
text = 'The following extra files have been found:<ul>';
response['response'].forEach(function(file) {
text += '<li>' + file + '</li>';
Copy link
Member

@LukasReschke LukasReschke Sep 22, 2016

Choose a reason for hiding this comment

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

Also worth escaping.

index.php Outdated
text += '</ul>';
var text = '';
if (typeof response['response'] === 'string') {
text = response['response'];
Copy link
Member

@LukasReschke LukasReschke Sep 22, 2016

Choose a reason for hiding this comment

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

Let's escape that.

index.php Outdated
} else {
text = 'The following places can not be written to:<ul>';
response['response'].forEach(function(file) {
text += '<li>' + file + '</li>';
Copy link
Member

Choose a reason for hiding this comment

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

Should be escaped.

index.php Outdated
text += '</ul>';
var text = '';
if (typeof response['response'] === 'string') {
text = response['response'];
Copy link
Member

Choose a reason for hiding this comment

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

Should be escaped.

@MorrisJobke
Copy link
Member Author

@LukasReschke I addressed your comments

@LukasReschke
Copy link
Member

Thanks, @MorrisJobke :)

@MorrisJobke
Copy link
Member Author

Together with nextcloud/server#1499 (and it's backports) this is what we need for our updater to work flawlessly - cc @nickvergessen @LukasReschke

@nickvergessen
Copy link
Member

Tested:

  1. Have additional files in place
  2. Start update
  3. See error that additional files exist
  4. Delete files
  5. Restart the update with the "Start update" button

Expected: List of additional files should be empty

Same "error" happens with "check write permissions".
The list should be cleared on a rerun

@nickvergessen
Copy link
Member

Other then that it worked fine.

@nickvergessen
Copy link
Member

👍

@MorrisJobke
Copy link
Member Author

  1. Restart the update with the "Start update" button

Good point. This should usually not be possible or otherwise better clear the current state of the UI.

@MorrisJobke
Copy link
Member Author

Same "error" happens with "check write permissions".
The list should be cleared on a rerun

Fixed.

  1. Restart the update with the "Start update" button

I fixed this properly. There is now a "Retry" button, that removes the warning of the last step and reruns it. The buttons are only shown, when they are actually needed.

@MorrisJobke
Copy link
Member Author

@LukasReschke @nickvergessen Could you have a look at the latest changes? thanks

@MorrisJobke
Copy link
Member Author

I just tested it with an 9.0.53 and this updater on IE11 and it works there fine too.

@LukasReschke LukasReschke merged commit 4aa04d6 into add-simplified-updater Sep 26, 2016
@LukasReschke LukasReschke deleted the refinements branch September 26, 2016 20:23
@MorrisJobke
Copy link
Member Author

🎉

@LukasReschke
Copy link
Member

RCs with the updater are built and available at:

=> https://download.nextcloud.com/server/prereleases/nextcloud-10.0.1RC2.tar.bz2
=> https://download.nextcloud.com/server/prereleases/nextcloud-9.0.54RC2.tar.bz2

@blizzz
Copy link
Member

blizzz commented Sep 26, 2016

Upgraded my instance to the 10.0.1RC2, worked fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants