Add a basic SelfUpdateCommand using phar-updater v1 #51

padraic · 2017-06-14T19:28:11Z

Implements #45

Assumes using Github releases to host a signed phar.

theofidry

First (not in-depth) review

theofidry · 2017-06-14T19:36:57Z

src/Console/Command/SelfUpdateCommand.php

+    {
+        $this
+            ->setName('self-update')
+            ->setDescription('Update php-scoper.phar to most recent stable build.')


can the name be changed? I mean the user may have renamed php-scoper.phar to something else e.g. php-scoper or scoper

Phar::running() should return the file path with the local name, I think.

theofidry · 2017-06-14T19:37:45Z

src/Console/Command/SelfUpdateCommand.php

+    protected function execute(InputInterface $input, OutputInterface $output)
+    {
+        $this->output = $output;
+        $this->version = $this->getApplication()->getVersion();


can this be done in the constructor?

theofidry · 2017-06-14T19:39:07Z

src/Console/Command/SelfUpdateCommand.php

+     */
+    private function getStableUpdater(): Updater
+    {
+        $updater = new Updater();


I didn't check the updater API but is there any reason why the strategy cannot be se in the updater constructor?

can we create the updater in the constructor or better use DI (for mocking in unit tests)?

theofidry · 2017-06-14T19:40:11Z

src/Console/Command/SelfUpdateCommand.php

+            $oldVersion = $updater->getOldVersion();
+
+            if ($result) {
+                $this->output->writeln('php-scoper has been updated.');


theofidry · 2017-06-14T19:40:39Z

src/Console/Command/SelfUpdateCommand.php

+            if ($result) {
+                $this->output->writeln('php-scoper has been updated.');
+                $this->output->writeln(sprintf(
+                    'Current version is: %s.',


<comment>%s</comment>

theofidry · 2017-06-14T19:40:44Z

src/Console/Command/SelfUpdateCommand.php

+                    $newVersion
+                ));
+                $this->output->writeln(sprintf(
+                    'Previous version was: %s.',


<comment>%s</comment>

theofidry · 2017-06-14T19:41:04Z

src/Console/Command/SelfUpdateCommand.php

+        } catch (\Exception $e) {
+            $this->output->writeln(sprintf('Error: %s', $e->getMessage()));
+        }
+        $this->output->write(PHP_EOL);


newLine() or writeln('')

theofidry · 2017-06-14T19:42:26Z

src/Console/Command/SelfUpdateCommand.php

+                ));
+            }
+        } catch (\Exception $e) {
+            $this->output->writeln(sprintf('Error: %s', $e->getMessage()));


is the rollback done in the updater in case of failure? If so we don't actually need to catch the exception as we'll have more information by throwing letting the Console handling the exception

The basic update flow is to backup the original phar (leaving original still in place), download the new phar to a temporary directory, validate it, and then copy it to the original phar location (overwrite). On error, the temporary copy would be deleted. The rollback is largely in the event that an update is valid, but broken or incompatible for any reason and you want the old version back.

padraic · 2017-06-14T19:56:35Z

@theofidry Yeah, still a bit of work to tidy this up. Will be tomorrow though :). The output can probably go to a logger somewhere too.

padraic · 2017-06-15T21:07:22Z

@theofidry Will look into testing tomorrow. Just cleaned it up and simplified.

theofidry · 2017-07-11T18:29:12Z

src/Console/Command/SelfUpdateCommand.php

+        if ($input->getOption('check')) {
+            $this->printAvailableUpdates();
+
+            return;


Null is converted to 0 status automatically isn't it? No harm being explicit though.

theofidry · 2017-07-11T18:29:53Z

src/Console/Command/SelfUpdateCommand.php

+    /**
+     * @return Updater
+     */
+    private function getStableUpdater(): Updater


createUpdater()?

Also I think the phpdoc is unneeded here

theofidry · 2017-07-11T18:31:04Z

src/Console/Command/SelfUpdateCommand.php

+            } else {
+                $this->logger->updateNotNeeded($oldVersion);
+            }
+        } catch (\Exception $e) {


What kind of error can we get? If it's a generic one maybe we should use Throwable instead?

Any day now I'll wake up and remember PHP 7 was released ;).

padraic · 2017-07-11T18:57:59Z

Made a few updates. For reference, the local box.json should have added the following new fields not in dist copy:

    "git-version": "git-version",
    "algorithm": "OPENSSL",
    "key": "/path/to/your/private.pem",
    "key-pass": true

Box version we use is pointing at Symfony/Console 3.x as an option, but the underlying commands are not updated to use the Question helper (still using Dialog). Adjusted our own vendor-bin/composer.json to fix.

I may temporarily hijack the 0.1.0 Github release for practical download/rollback testing tomorrow.

The phar will emit a semantic version based off of box's git-version, so it will be one of:

x.y.z (typical stable version when phar is build to a tagged commit.
x.y.z+15-g0818afa (where + marks anything after as metadata, 15 is the commit count since previous tagged commit, and 0818afa is the current short commit hash prefix with g by box.

theofidry · 2017-07-11T19:24:46Z

Box version we use is pointing at Symfony/Console 3.x as an option, but the underlying commands are not updated to use the Question helper (still using Dialog). Adjusted our own vendor-bin/composer.json to fix.

I'm not following. You mean it allows symfony/console 3.x but actually the code is still using 2.x code which as been removed in 3.x?

Nevermind, actually saw that which confirms it. Seems fine by me really 👍

I'm must stay I'm still not sure how the phar updater works exactly as I'm only used to the Composer one.

padraic · 2017-07-11T19:40:00Z

@theofidry That's the problem - still using Symfony 2's Dialog helper in the background.

The updater works a lot like Composer's - I doubt there's a huge difference between them at the basic level. The differences are all extras to the basic workflow: phar signature support, the strategies (all essentially varying methods of where/how to check for updates, and where to download any updates).

padraic · 2017-07-13T20:31:12Z

@theofidry All wrapped up here!

theofidry

Some minor comments and I think we're good

theofidry · 2017-07-13T21:25:20Z

src/Console/Command/SelfUpdateCommand.php

+                $this->logger->updateNotNeeded($oldVersion);
+            }
+        } catch (\Throwable $e) {
+            $this->logger->error($e);


we're not failing here?

Mostly, it's just to add some preceding text to assure users the phar was not replaced. We could let the Throwable just fall through, but I always find that a bit untidy.

Not sure about this though as we rule out any way for the user to know why it failed. I think throwing the throwable after logging the message would be fine

theofidry · 2017-07-13T21:26:34Z

src/Console/Command/SelfUpdateCommand.php

+
+    private function getLocalPharName(): string
+    {
+        return basename(\PHAR::running());


This command doesn't work without a PHAR, but can we ensure it fails gracefully?

Can you clarify? The SelfUpdateCommand only ever exists in a PHAR, so the above running() call should always succeed.

theofidry · 2017-07-13T21:27:18Z

src/Logger/UpdateConsoleLogger.php

+
+    public function updateSuccess(string $newVersion, string $oldVersion)
+    {
+        $this->io->writeln('PHP-Scoper has been updated.');


with SymfonyStyle you also have success(), error() and such for some fancy styling

theofidry · 2017-07-13T22:20:29Z

Hum Travis is now failing because of the missing pubkey...

padraic · 2017-07-14T09:26:29Z

Hmm, phar-updater checks it up front instead of waiting for a request for a pub key to run checks: https://github.com/humbug/phar-updater/blob/master/src/Updater.php#L112

It should be changed in the updater package for these scenarios (unsigned testing). I'll open an issue there. In the meantime, we can probably suppress that exception. For actual updating in RL, the phar itself won't even run if the pubkey were missing.

Add public key to .gitignore

Misc fixes; Require self-update dev-master for now

padraic · 2017-07-15T13:12:10Z

@theofidry All comments should be addressed now. Made one change to makefile so we ignore, only during tests, any local box.json (e.g. with privkey config) and use the dist copy instead.

theofidry

Looks good to me. Could you just take a look at Scrutinizer before merging to see if the introduced issues are relevant or not?

padraic · 2017-07-15T21:19:48Z

@theofidry Made a couple of changes. The remainder on Scrutinizer we can't fix from outside phar-updater.

theofidry reviewed Jun 14, 2017

View reviewed changes

padraic force-pushed the feature/self-update branch from d618d36 to 97712e7 Compare June 15, 2017 20:07

padraic force-pushed the feature/self-update branch from 5c5a604 to 4a0370f Compare June 19, 2017 17:48

padraic force-pushed the feature/self-update branch from 4a0370f to f4a8cef Compare July 11, 2017 17:17

theofidry reviewed Jul 11, 2017

View reviewed changes

padraic changed the title ~~[WIP] Add a basic SelfUpdateCommand using phar-updater v1~~ Add a basic SelfUpdateCommand using phar-updater v1 Jul 13, 2017

padraic force-pushed the feature/self-update branch from aa19456 to 1c1b9f2 Compare July 13, 2017 20:29

theofidry reviewed Jul 13, 2017

View reviewed changes

padraic added 14 commits July 15, 2017 14:10

Add a basic SelfUpdateCommand using phar-updater v1

39793e0

CS fixes

9695621

Updates + add console logger

6b8e784

CS fix and add text styles

45b5b2c

Simplify command

f93fd76

CS fix

97c5af7

Fix box dependency missing Dialog feature;

3f99e77

Add public key to .gitignore

Set and update from a semantic version format;

85c31cc

Misc fixes; Require self-update dev-master for now

Remove leading 'g' of short commit hash

58b983c

Step too far - self-updater requires an 8char count for short commit

f8b6573

Update semantic version; add back versioned phar-updater

ba29717

Add ignore entries for phar temp/old files

7a845e8

Minor change

2d32a01

Use Throwable because...PHP7 dummy

54abaaf

padraic added 4 commits July 15, 2017 14:10

Missed one!

04e89ee

Address @theofidry's final comments

6623a86

Ignore custom/local box.json during tests

72e113f

CS fix

27d97ac

padraic force-pushed the feature/self-update branch from 190d945 to 27d97ac Compare July 15, 2017 13:10

theofidry approved these changes Jul 15, 2017

View reviewed changes

Remove dead code; add command to code cov ignore list

7251cb4

theofidry merged commit 557dce1 into humbug:master Jul 15, 2017

padraic deleted the feature/self-update branch July 15, 2017 21:33

theofidry mentioned this pull request Jul 20, 2017

Add self-update command #45

Closed

Add a basic SelfUpdateCommand using phar-updater v1 #51

Add a basic SelfUpdateCommand using phar-updater v1 #51

Conversation

padraic commented Jun 14, 2017 • edited Loading

theofidry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padraic commented Jun 14, 2017

padraic commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padraic commented Jul 11, 2017 • edited Loading

theofidry commented Jul 11, 2017

padraic commented Jul 11, 2017

padraic commented Jul 13, 2017

theofidry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Jul 13, 2017

padraic commented Jul 14, 2017

padraic commented Jul 15, 2017

theofidry left a comment

Choose a reason for hiding this comment

padraic commented Jul 15, 2017

padraic commented Jun 14, 2017 •

edited

Loading

padraic commented Jul 11, 2017 •

edited

Loading